Skip to content

Try caching playwright browsers download #33557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .azure/pipelines/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ variables:
value: .NETCORE
- name: _DotNetValidationArtifactsCategory
value: .NETCORE
- name: PLAYWRIGHT_BROWSERS_PATH
value: $(System.DefaultWorkingDirectory)/.tools/playwright
- name: PostBuildSign
value: true
- ${{ if and(ne(variables['System.TeamProject'], 'public'), notin(variables['Build.Reason'], 'PullRequest')) }}:
Expand Down Expand Up @@ -659,6 +661,15 @@ stages:
beforeBuild:
- powershell: "& ./src/Servers/IIS/tools/UpdateIISExpressCertificate.ps1; & ./src/Servers/IIS/tools/update_schema.ps1"
displayName: Setup IISExpress test certificates and schema
- task: Cache@2
inputs:
key: 'pw | "$(Agent.OS)" | eng/versions.props'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eng/Versions.props doesn't list the npm package version for Playwright. I'm not sure where the version is stored. @javiercn❔ If we just use the latest, caching would be a Bad Thing:tm:.

path: $(PLAYWRIGHT_BROWSERS_PATH)
cacheHitVar: CACHE_PLAYWRIGHT_HIT
displayName: Cache playwright browsers
- script: npm i -D playwright
condition: ne(variables['CACHE_PLAYWRIGHT_HIT'], 'true')

artifacts:
- name: Windows_Test_Dumps
path: artifacts/dumps/
Expand Down Expand Up @@ -727,6 +738,15 @@ stages:
agentOs: Windows
timeoutInMinutes: 240
steps:
- task: Cache@2
inputs:
key: 'pw | "$(Agent.OS)" | eng/versions.props'
path: $(PLAYWRIGHT_BROWSERS_PATH)
cacheHitVar: CACHE_PLAYWRIGHT_HIT
displayName: Cache playwright browsers
- script: npm i -D playwright
condition: ne(variables['CACHE_PLAYWRIGHT_HIT'], 'true')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the Helix test job need Playwright❔ Suggest leaving this job alone

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the quarantined version of helix will need it, and eventually the real one too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we do Playwright installation on the Helix agents❔ I could understand this if the installations were uploaded from the build agents.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it does it both on the helix machines but also as part of local builds/tests since the developer workflow is a thing too, generally it doesn't look like our builds are factored in a way that makes it easy to optionally install dependencies. I had to do some magic with conditional compilation for that to work on helix, so its possible but currently we aren't setup to easily stop installing playwright on builds


# Build the shared framework
- script: ./eng/build.cmd -ci -nobl -all -pack -arch x64
/p:CrossgenOutput=false /p:ASPNETCORE_TEST_LOG_DIR=artifacts/log $(_InternalRuntimeDownloadArgs)
Expand Down