test: use puppeteer in integration tests and to download correct chromedriver

[gregmagolan]

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 <== @kyliau

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

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: integration test changes

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

[gkalpak]

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_ARG environment variable.

[gkalpak]

Getting there 😃
Good call moving the AIO stuff to a different PR 👍

[gkalpak]

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)?

[gregmagolan]

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

[gregmagolan]

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?

[gregmagolan]

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.

[gkalpak]

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 😁)

[gregmagolan]

Cool. yarn could break it in the future but it will be obvious as the @npm//webdriver-manager label will no longer exist.

[gkalpak]

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?

[gregmagolan]

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.

[gregmagolan]

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.

[gkalpak]

Could we switch to the non-browser CircleCI docker images in some jobs (e.g. the integration one) now that we use headless Chrome?

[gregmagolan]

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

[filipesilva]

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.

[filipesilva]

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".

[gregmagolan]

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.

[gregmagolan]

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.

[gregmagolan]

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.

Out of date

[josephperrott]

LGTM, just a couple nits

[angular-automatic-lock-bot]

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.}