Skip to content

CI Test for #35049#35102

Closed
gregmagolan wants to merge 4 commits intoangular:masterfrom
gregmagolan:puppeteer_integration_tests_aio
Closed

CI Test for #35049#35102
gregmagolan wants to merge 4 commits intoangular:masterfrom
gregmagolan:puppeteer_integration_tests_aio

Conversation

@gregmagolan
Copy link
Copy Markdown
Contributor

@gregmagolan gregmagolan commented Feb 1, 2020

Testing CI for #35049. Turns on aio monitoring jobs to make sure they are not broken.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@pullapprove pullapprove bot requested a review from filipesilva February 1, 2020 18:13
@gregmagolan gregmagolan changed the title Puppeteer integration tests aio aio build switched to puppeteer Feb 1, 2020
@gregmagolan gregmagolan changed the title aio build switched to puppeteer test: use puppeteer in aio build instead to remove CI_CHROMEDRIVER_VERSION_ARG Feb 1, 2020
@mhevery mhevery added the area: build & ci Related the build and CI infrastructure of the project label Feb 4, 2020
@ngbot ngbot bot added this to the needsTriage milestone Feb 4, 2020
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how come this one uses a different pattern than others? I actually like this pattern better because it's more explicit and more obvious, is there a downside to using it throughout the repo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

karma requires CHROME_BIN as there is no other way to specify the bin for karma-chrome-launcher. protractor config allows you to set the binary like this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't we then also remove the browsers-executor executor from the circleci config and replace it with the default executor?

Copy link
Copy Markdown
Contributor Author

@gregmagolan gregmagolan Feb 7, 2020

Choose a reason for hiding this comment

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

The last holdout is still integration/bazel-schematics which I didn't want to touch yet as it is more complicated but all other tests including aio should be able to go to default-executor now. Good call.

@gregmagolan gregmagolan changed the title test: use puppeteer in aio build instead to remove CI_CHROMEDRIVER_VERSION_ARG CI Test for #35049 Feb 7, 2020
…medriver

This means integration tests no longer need to depend on a $CI_CHROMEDRIVER_VERSION_ARG environment variable to specify which chromedriver version to download to match the locally installed chrome. This was bad DX and not having it specified was not reliable as webdriver-manager would not always download the chromedriver version to work with the locally installed chrome.

webdriver-manager update --gecko=false --standalone=false $CI_CHROMEDRIVER_VERSION_ARG is now replaced with node webdriver-manager-update.js in the root package.json, which checks which version of chrome puppeteer has come bundled with & downloads informs webdriver-manager to download the corresponding chrome driver version.

Integration tests now use "webdriver-manager": "file:../../node_modules/webdriver-manager" so they don't have to waste time calling webdriver-manager update in postinstall

"// resolutions": "Ensure a single version of webdriver-manager which comes from root node_modules that has already run webdriver-manager update",
"resolutions": {
"**/webdriver-manager": "file:../../node_modules/webdriver-manager"
}
This should speed up each integration postinstall by a few seconds.

Further, integration test package.json files link puppeteer via file:../../node_modules/puppeteer which is the ideal situation as the puppeteer post-install won't download chrome if it is already downloaded. In CI, since node_modules is cached it should not need to download Chrome either unless the node_modules cache is busted.

NB: each version of puppeteer comes bundles with a specific version of chrome. Root package.json & yarn.lock currently pull down puppeteer 2.1.0 which comes with chrome 80. See https://github.com/puppeteer/puppeteer#q-which-chromium-version-does-puppeteer-use for more info.

Only two references to CI_CHROMEDRIVER_VERSION_ARG left in integration tests at integration/bazel-schematics/test.sh which I'm not entirely sure how to get rid of it

Use a lightweight puppeteer=>chrome version mapping instead of launching chrome and calling browser.version()

Launching puppeteer headless chrome and calling browser.version() was a heavy-handed approach to determine the Chrome version. A small and easy to update mappings file is a better solution and it means that the `yarn install` step does not require chrome shared libs available on the system for its postinstall step
@gregmagolan
Copy link
Copy Markdown
Contributor Author

#35049 landed

@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: build & ci Related the build and CI infrastructure of the project cla: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants