test: use puppeteer in integration tests and to download correct chromedriver#35049
test: use puppeteer in integration tests and to download correct chromedriver#35049gregmagolan wants to merge 3 commits intoangular:masterfrom gregmagolan:puppeteer_integration_tests
Conversation
integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Yay! One step closer to deterministic builds 🎉 💯
Follow-up tasks (mostly a note to self):
- Swtch all AIO scripts to the puppeteer setup (e.g.
test-a11y-score,test-pwa-score). - Switch all docs examples scripts to the puppeteer setup.
- Get rid of the
CI_CHROMEDRIVER_VERSION_ARGenvironment variable.
gkalpak
left a comment
There was a problem hiding this comment.
Getting there 😃
Good call moving the AIO stuff to a different PR 👍
package.json
Outdated
There was a problem hiding this comment.
I don't get this. Were there multiple versions (I only see one). Is it to ensure that the version is hoisted by yarn (in case other dependencies transitively depend on another version of webdriver-manager in the future)?
There was a problem hiding this comment.
I was under the impression that webdriver-manager needed to be hoisted to work but since there is a protractor webdriver-manager bin entry then its not. Thanks George
There was a problem hiding this comment.
Actually. Circling back to this. The integration tests are relying on webdriver-manager to be found at ../../node_modules/webdriver-manager so I think we need to have "**/webdriver-manager" in resolutions to ensure that there is only one version that will get hoisted incase a dep is added in the future that also transitively depends on webdriver-manager of a different version.
Is there another way to ensure this?
There was a problem hiding this comment.
Further to that, when npm_integration_test is brought in, it will depend on the @npm//webdriver-manager label so we will depend on the hoisting.
There was a problem hiding this comment.
I don't know of another way to ensure that. (I am not even sure this way ensures it, but it is probably our best bet 😁)
There was a problem hiding this comment.
Cool. yarn could break it in the future but it will be obvious as the @npm//webdriver-manager label will no longer exist.
.circleci/config.yml
Outdated
There was a problem hiding this comment.
Why is this change needed? Aren't the CircleCI images we use to run those tests providing the necessary deps? How did they work before?
There was a problem hiding this comment.
In short, there were not needed before as Chrome was not being run in the job but now that the postinstall webdriver-manager update script runs puppeteers Chrome in headless to find out it's version the shared libs are needed in all containers.
Added a comment above as to why as realized that was missing:
These are now needed in all containers as the webdriver-manager update script which is run in the root postinstall
now starts puppeteer's Chrome in headless to check its version so the correct chromedriver is downloaded. See /scripts/webdriver-manager-update.js.
There was a problem hiding this comment.
And no, the base circleci/node image does not have the required shared libs for Chrome to run. The circleci/node-browsers image does but it also includes the browsers which would add more time to each job to setup the environment than doing this apt-get here.
There was a problem hiding this comment.
Could we switch to the non-browser CircleCI docker images in some jobs (e.g. the integration one) now that we use headless Chrome?
There was a problem hiding this comment.
Almost. the integration/bazel-schematics test is the last that still uses local chrome but I'll need some help from @kyliau to remove that dependency so likely to be a follow up PR
There was a problem hiding this comment.
We can partially work around this by having an alternate method of looking up the chrome version. Puppeteer includes in its package.json a field that indicates the chrome revision, and from that we can get to the version number somehow. I think there's a online API for that? I remember seeing it once, but can't find it now.
This still wouldn't address the fact we're installing puppeteer/webdriver/chrome deps on jobs that never use it. It would just help to only install chrome deps on jobs that do use it. It would also make the setup a bit more complicated because now those jobs would need to explicitly install these deps.
But if we'd get to the point where we're saying "this job needs chrome deps for chromedriver", why not go all the way and say "this job needs chromedriver" instead, and only then update webdriver?
I suppose the downside is that local installs are not privy to this logic. But we already have an answer to that too: ./.circleci/env.sh contains variables only present in circleci. Maybe that can be leveraged to only run the webdriver update on jobs that need it, and to always run it outside circleci.
There was a problem hiding this comment.
Reviewing this approach makes me think it's somewhat worse than just using the circleci images with chrome when needed. Instead of installing chrome deps we could say "if this environment has chrome, install webdriver".
There was a problem hiding this comment.
With puppeteer in the root package.json, its postinstall step to download chrome will happen in the setup job as well and it is actually quite slow. The webdriver update is fast in comparison but doing it in the postinstall of the root package.json simplifies the setup for all integration tests so they can just depend on ../../node_modules/puppeteer && ../../node_modules/webdriver-manager and puppeteer won't download Chrome since it is already downloaded and webdriver-manager update won't need to be called. IMO that's enough reason to do this in the root package.json in order to simplify the integration tests and save 10s or so for each one... although the webdriver-manager update is quite fast in comparison to the puppeteer post-install so it could still be done in each integration test, however, these will soon run in the the same CircleCI test job as everything else.
With #33927, all integration tests will be run in the test && test_ivy_aot jobs by Bazel in CircleCI and the legacy integration tests job & shell script will be removed in a follow up PR. Eventually some or all of the aio jobs could get moved to run under bazel as well using npm_integration_test so with moving to a setup where bazel test ... can test everything it makes more sense for the root package.json to run the webdriver-update.
NB: for cross-platform RBE, the integration tests puppeteer postinstall will still download chrome since it will need the linux Chrome for the remove executor while the repository_rule (which always runs on the local host machine) would have downloaded the OSX binaries.
All that being said... if there is an easy way to get the Chrome version from the puppeteer package.json with an API to map the version number or to vendor in a mapping file from somewhere so it is available locally then that would mean we wouldn't need to have these shared libs installed in the setup job but could move it to only the jobs that actually need to run the puppeteer (or rules_webtesting) provisioned Chrome for testing. It does seem like a heavy-handed approach to launch chrome in the yarn postinstall just to find out what version it is.
There was a problem hiding this comment.
Ok. After some thought I opted for a very simple mapping up puppeteer semver version to Chrome version:
// Mapping of puppeteer releases to their default Chrome version
// derived from https://github.com/puppeteer/puppeteer/blob/master/docs/api.md.
// The puppeteer package.json file the Chrome revision such as
// "chromium_revision": "722234" but this does not map easily to the Chrome
// so we use this mapping here instead.
module.exports = {
"2.1.0": "80.0.3987.0",
"2.0.0": "79.0.3942.0",
"1.20.0": "78.0.3882.0",
"1.19.0": "77.0.3803.0",
"1.17.0": "76.0.3803.0",
"1.15.0": "75.0.3765.0",
"1.13.0": "74.0.3723.0",
"1.12.2": "73.0.3679.0",
};
This will be easy to maintain (maybe it should be upstreamed to puppeteer) and when puppeteer is updated to a new version the webdriver-manager-update script will error out and prompt you to update the mappings file.
There was a problem hiding this comment.
This means that the root yarn install postinstall setup no longer needs to have chrome shared libs available because it no longer needs to launch headless chrome.
|
aio changes included back into this PR at Igor's request after all the review comments & Igor reviewed both. #35102 now just testing CI job for this PR that only run on master |
|
@gkalpak I switch the PWA score test to use protractor. Couldn't figure out the doc examples so will leave that to you :) |
IgorMinar
left a comment
There was a problem hiding this comment.
this looks good to me now. Thanks @gregmagolan for working through all the review feedback.
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
|
let's see if @filipesilva or @gkalpak have anything else to add. I believe that all their feedback has been addressed. |
filipesilva
left a comment
There was a problem hiding this comment.
LGTM, thanks! I left a few non blocking comments, but it's up to you if they are a good suggestion or not.
There was a problem hiding this comment.
Maybe this comment could also be in integration/README.md instead of repeated on karma configs.
There was a problem hiding this comment.
Seems to me like its better in this file even if there is some repetition as in the README.md you'd loose the context
There was a problem hiding this comment.
Maybe this comment could also be in integration/README.md instead of repeated on package.json.
josephperrott
left a comment
There was a problem hiding this comment.
LGTM, just a couple nits
…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
…medriver (#35049) 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 PR Close #35049
This is a follow-up to angular#35049 with a few minor fixes related to using the browser provided by `puppeteer` to run tests. Included fixes: - Make the `webdriver-manager-update.js` really portable. (Previously, it needed to be run from the directory that contained the `node_modules/` directory. Now, it can be executed from a subdirectory and will correctly resolve dependencies.) - Use the `puppeteer`-based setup in AIO unit and e2e tests to ensure that the downloaded ChromeDriver version matches the browser version used in tests. - Use the `puppeteer`-based setup in the `aio_monitoring_stable` CI job (as happens with `aio_monitoring_next`). - Use the [recommended way][1] of getting the browser port when using `puppeteer` with `lighthouse` and avoid hard-coding the remote debugging port (to be able to handle multiple instances running concurrently). [1]: https://github.com/GoogleChrome/lighthouse/blame/51df179a0/docs/puppeteer.md#L49
… tests In angular#35049, integration and AIO tests were changed to use the browser provided by `puppeteer` in tests. This commit switches the docs examples tests to use the same setup. IMPLEMENTATION NOTE: The examples are used to create ZIP archives that docs users can download to experiment with. Since we want the downloaded projects to resemble an `@angular/cli` generated project, we do not want to affect the project's Protractor configuration in order to use `puppeteer`. To achieve this, a second Protractor configuration is created (which is ignored when creating the ZIP archives) that extends the original one and passes the approperiate arguments to use the browser provided by `puppeteer`. This new configuration (`protractor-puppeteer.conf.js`) is used when running the docs examples tests (on CI or locally during development).
This is a follow-up to #35049 with a few minor fixes related to using the browser provided by `puppeteer` to run tests. Included fixes: - Make the `webdriver-manager-update.js` really portable. (Previously, it needed to be run from the directory that contained the `node_modules/` directory. Now, it can be executed from a subdirectory and will correctly resolve dependencies.) - Use the `puppeteer`-based setup in AIO unit and e2e tests to ensure that the downloaded ChromeDriver version matches the browser version used in tests. - Use the `puppeteer`-based setup in the `aio_monitoring_stable` CI job (as happens with `aio_monitoring_next`). - Use the [recommended way][1] of getting the browser port when using `puppeteer` with `lighthouse` and avoid hard-coding the remote debugging port (to be able to handle multiple instances running concurrently). [1]: https://github.com/GoogleChrome/lighthouse/blame/51df179a0/docs/puppeteer.md#L49 PR Close #35381
… tests (#35381) In #35049, integration and AIO tests were changed to use the browser provided by `puppeteer` in tests. This commit switches the docs examples tests to use the same setup. IMPLEMENTATION NOTE: The examples are used to create ZIP archives that docs users can download to experiment with. Since we want the downloaded projects to resemble an `@angular/cli` generated project, we do not want to affect the project's Protractor configuration in order to use `puppeteer`. To achieve this, a second Protractor configuration is created (which is ignored when creating the ZIP archives) that extends the original one and passes the approperiate arguments to use the browser provided by `puppeteer`. This new configuration (`protractor-puppeteer.conf.js`) is used when running the docs examples tests (on CI or locally during development). PR Close #35381
This is a follow-up to #35049 with a few minor fixes related to using the browser provided by `puppeteer` to run tests. Included fixes: - Make the `webdriver-manager-update.js` really portable. (Previously, it needed to be run from the directory that contained the `node_modules/` directory. Now, it can be executed from a subdirectory and will correctly resolve dependencies.) - Use the `puppeteer`-based setup in AIO unit and e2e tests to ensure that the downloaded ChromeDriver version matches the browser version used in tests. - Use the `puppeteer`-based setup in the `aio_monitoring_stable` CI job (as happens with `aio_monitoring_next`). - Use the [recommended way][1] of getting the browser port when using `puppeteer` with `lighthouse` and avoid hard-coding the remote debugging port (to be able to handle multiple instances running concurrently). [1]: https://github.com/GoogleChrome/lighthouse/blame/51df179a0/docs/puppeteer.md#L49 PR Close #35381
… tests (#35381) In #35049, integration and AIO tests were changed to use the browser provided by `puppeteer` in tests. This commit switches the docs examples tests to use the same setup. IMPLEMENTATION NOTE: The examples are used to create ZIP archives that docs users can download to experiment with. Since we want the downloaded projects to resemble an `@angular/cli` generated project, we do not want to affect the project's Protractor configuration in order to use `puppeteer`. To achieve this, a second Protractor configuration is created (which is ignored when creating the ZIP archives) that extends the original one and passes the approperiate arguments to use the browser provided by `puppeteer`. This new configuration (`protractor-puppeteer.conf.js`) is used when running the docs examples tests (on CI or locally during development). PR Close #35381
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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_ARGis now replaced withnode webdriver-manager-update.jsin 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 postinstallThis should speed up each integration postinstall by a few seconds.
Further, integration test package.json files link puppeteer via
file:../../node_modules/puppeteerwhich 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.shwhich I'm not entirely sure how to get rid of it <== @kyliauPR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information