build: several minor fixes related to using `puppeteer` by gkalpak · Pull Request #35381 · angular/angular

gkalpak · 2020-02-12T14:49:15Z

This PR is a follow-up to #35049 and contains several fixes related to using the browser provided by puppeteer to run tests. It contains the following commits:

build: several minor fixes related to using puppeteer

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

build(docs-infra): use puppeteer to get a browser for docs examples tests

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

ci: get rid of the CI_CHROMEDRIVER_VERSION_ARG env var

Previously, we needed to manually specify a ChromeDriver version to download on CI that would be compatible with the browser version provided by the docker image used to run the tests. This was kept in the CI_CHROMEDRIVER_VERSION_ARG environment variable.

With recent commits, we use the browser provided by puppeteer and can determine the correct ChromeDriver version programmatically. Therefore, we no longer need the CI_CHROMEDRIVER_VERSION_ARG environment variable.

NOTE:
There is still one place (the bazel-schematics integration project) where a hard-coded ChromeDriver version is necessary. Since I am not sure what is the best way to refactor the tests to not rely on a hard-coded version, I left it as a TODO for a follow-up PR.

mary-poppins · 2020-02-12T14:59:28Z

You can preview 2a973b2 at https://pr35381-2a973b2.ngbuilds.io/.

mary-poppins · 2020-02-12T17:15:04Z

You can preview ccfcce9 at https://pr35381-ccfcce9.ngbuilds.io/.

mary-poppins · 2020-02-12T17:36:08Z

You can preview 6026d09 at https://pr35381-6026d09.ngbuilds.io/.

mary-poppins · 2020-02-12T17:58:48Z

You can preview 6302c41 at https://pr35381-6302c41.ngbuilds.io/.

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

Previously, we needed to manually specify a ChromeDriver version to download on CI that would be compatible with the browser version provided by the docker image used to run the tests. This was kept in the `CI_CHROMEDRIVER_VERSION_ARG` environment variable. With recent commits, we use the browser provided by `puppeteer` and can determine the correct ChromeDriver version programmatically. Therefore, we no longer need the `CI_CHROMEDRIVER_VERSION_ARG` environment variable. NOTE: There is still one place (the `bazel-schematics` integration project) where a hard-coded ChromeDriver version is necessary. Since I am not sure what is the best way to refactor the tests to not rely on a hard-coded version, I left it as a TODO for a follow-up PR.

mary-poppins · 2020-02-13T15:26:48Z

You can preview 6dd6ffe at https://pr35381-6dd6ffe.ngbuilds.io/.

gregmagolan · 2020-02-13T16:57:59Z

.circleci/env.sh

-# `.circleci/config.yml`. See http://chromedriver.chromium.org/downloads for a list of versions.
-# This variable is intended to be passed as an arg to the `webdriver-manager update` command (e.g.
-# `"postinstall": "webdriver-manager update $CI_CHROMEDRIVER_VERSION_ARG"`).
-setPublicVar CI_CHROMEDRIVER_VERSION_ARG "--versions.chrome 79.0.3945.130";


I think we still need this for one last test which is the integration/bazel-schematics. Once that is updated to puppeteer we can finally kill it.

Ahh. I see you hard-coded it there instead 👍 👍

gregmagolan · 2020-02-13T16:59:16Z

aio/karma.conf.js

+      },
    },
-    browsers: [process.env['CI'] ? 'ChromeHeadlessNoSandbox' : 'Chrome'],
+    browsers: ['ChromeHeadlessNoSandbox'],


For local testing I suppose you're no relying on the browser popping up and if you need it you can always change this temporarily?

Yeah, exactly. I still needed to pass some flags for the puppeteer-provided Chrome to work locally (at least on Windows) and I realized we hardly ever need the browser. For debugging you can always open a browser and point it to the Karma URL (or change this config temporarily as you mentioned).

gregmagolan · 2020-02-13T16:59:42Z

aio/scripts/audit-web-app.js

-    args: ['--remote-debugging-port=9222']});
-  flags.port = browser.port;
+  const browser = await puppeteer.launch();
+  flags.port = (new URL(browser.wsEndpoint())).port;


gregmagolan · 2020-02-13T17:06:36Z

scripts/webdriver-manager-update.js


-const isWindows = process.platform === 'win32';
-const result = spawnSync(`${process.cwd()}/node_modules/.bin/webdriver-manager${isWindows ? '.cmd' : ''}`, args, {stdio: 'inherit'});
+const result = spawnSync('yarn', args, {shell: true, stdio: 'inherit'});


I was avoiding yarn as in a follow up PR i'll run the aio build in npm_integration_test so under Bazel I want to ensure that the correct version of yarn is used. However, under Bazel I think yarn will be on the path and it will redirect to the Bazel provisioned yarn to be hermetic.

However, this may now be more an issue outside of Bazel now since we're relying on the root .yarnrc to use yarn 1.21.1 and in this context there will be no .yarnrc in the cwd. + @josephperrott 👀

I think this will end up using the locally installed yarn now which may be out of date

Yarn is clever enough to look at parent directories and find the closest .yarnrc (if any). So, it will use our vendored version regardless of the directory.

If this turns out to be a problem for bazel, we can use require.resolve() to find the relative path to the closest node_modules/ directory (but yarn was easier for now and from what I understand it might work in bazel too).

That's true for .yarnrc as I had to sift through that code to get yarn_install to honor .yarnrc and the yarn-path. However, there is special treatment for the yarn-path setting as it is a start-up setting; it may only be used if it is in a .yarnrc in the cwd as otherwise you can get into infinite recursion situations. You may be right however but should verify that is the case.

Confirmed that it picks up the version from .yarnrc as desired.

👍 Thanks @josephperrott

gregmagolan

LGTM. Will be some conflict with #33927 but this will probably land first tho so I'll handle the conflicts on rebase.

gregmagolan · 2020-02-16T17:49:25Z

After #33927 and this land I should be able to run aio in an npm_integration_test :)

josephperrott

LGTM

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

Previously, we needed to manually specify a ChromeDriver version to download on CI that would be compatible with the browser version provided by the docker image used to run the tests. This was kept in the `CI_CHROMEDRIVER_VERSION_ARG` environment variable. With recent commits, we use the browser provided by `puppeteer` and can determine the correct ChromeDriver version programmatically. Therefore, we no longer need the `CI_CHROMEDRIVER_VERSION_ARG` environment variable. NOTE: There is still one place (the `bazel-schematics` integration project) where a hard-coded ChromeDriver version is necessary. Since I am not sure what is the best way to refactor the tests to not rely on a hard-coded version, I left it as a TODO for a follow-up PR. 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

Previously, we needed to manually specify a ChromeDriver version to download on CI that would be compatible with the browser version provided by the docker image used to run the tests. This was kept in the `CI_CHROMEDRIVER_VERSION_ARG` environment variable. With recent commits, we use the browser provided by `puppeteer` and can determine the correct ChromeDriver version programmatically. Therefore, we no longer need the `CI_CHROMEDRIVER_VERSION_ARG` environment variable. NOTE: There is still one place (the `bazel-schematics` integration project) where a hard-coded ChromeDriver version is necessary. Since I am not sure what is the best way to refactor the tests to not rely on a hard-coded version, I left it as a TODO for a follow-up PR. PR Close #35381

@gkalpak

…matics (#33927) This means we don't need the action_env. Borrowed from @gkalpak's changes in #35381. PR Close #33927

@gkalpak

…matics (#33927) This means we don't need the action_env. Borrowed from @gkalpak's changes in #35381. PR Close #33927

… ZIP archives In angular#35381, a new Protractor config file was introduced in docs examples, `protractor-puppeteer.conf.js`, that was only supposed to be used on CI and not be shipped with the ZIP archives provided for users to download and experiment with the docs examples locally. The logic to ignore the `protractor-puppeteer.conf.js` file was incorrect, resulting in the file being retained in some examples (e.g. [universal][1]). The problem was not immediately obvious, because most examples explicitly specify all `**/*.js` files as ignored, but for other examples the file was retained in the ZIP archive. This commit fixes the logic to ensure the file is excluded from all docs examples ZIP archives. [1]: https://v9.angular.io/generated/zips/universal/universal.zip

angular-automatic-lock-bot · 2020-03-20T16:06:46Z

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

googlebot added the cla: yes label Feb 12, 2020

gkalpak force-pushed the ci-aio-puppeteer branch from 2a973b2 to ccfcce9 Compare February 12, 2020 17:04

gkalpak force-pushed the ci-aio-puppeteer branch from ccfcce9 to 6026d09 Compare February 12, 2020 17:26

gkalpak force-pushed the ci-aio-puppeteer branch from 6026d09 to 6302c41 Compare February 12, 2020 17:49

kara added the area: build & ci Related the build and CI infrastructure of the project label Feb 13, 2020

ngbot bot added this to the needsTriage milestone Feb 13, 2020

gkalpak added 3 commits February 13, 2020 16:46

gkalpak force-pushed the ci-aio-puppeteer branch from 6302c41 to 6dd6ffe Compare February 13, 2020 15:15

gkalpak changed the title ~~WIP - Use puppeteer in docs-infra~~ build: several minor fixes related to using puppeteer Feb 13, 2020

gkalpak added comp: docs-infra target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels Feb 13, 2020

gregmagolan reviewed Feb 13, 2020

View reviewed changes

gkalpak marked this pull request as ready for review February 13, 2020 18:46

gregmagolan mentioned this pull request Feb 14, 2020

build: npm_integration_test && angular_integration_test #33927

Closed

14 tasks

gregmagolan self-requested a review February 16, 2020 17:46

gregmagolan approved these changes Feb 16, 2020

View reviewed changes

gkalpak requested a review from josephperrott February 17, 2020 12:49

josephperrott approved these changes Feb 18, 2020

View reviewed changes

alxhub closed this in ab8199f Feb 18, 2020

gkalpak deleted the ci-aio-puppeteer branch February 18, 2020 21:03

mhevery pushed a commit that referenced this pull request Feb 24, 2020

build: remove CI_CHROMEDIRVER_VERSION_ARG from integration/bazel-sche…

561d402

…matics (#33927) This means we don't need the action_env. Borrowed from @gkalpak's changes in #35381. PR Close #33927

mhevery pushed a commit that referenced this pull request Feb 24, 2020

build: remove CI_CHROMEDIRVER_VERSION_ARG from integration/bazel-sche…

9efd7af

…matics (#33927) This means we don't need the action_env. Borrowed from @gkalpak's changes in #35381. PR Close #33927

gkalpak mentioned this pull request Mar 11, 2020

build(docs-infra): do not include CI-specific config in docs examples ZIP archives #36018

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 20, 2020

Conversation

gkalpak commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mary-poppins commented Feb 12, 2020

Uh oh!

mary-poppins commented Feb 12, 2020

Uh oh!

mary-poppins commented Feb 12, 2020

Uh oh!

mary-poppins commented Feb 12, 2020

Uh oh!

mary-poppins commented Feb 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregmagolan Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregmagolan left a comment

Choose a reason for hiding this comment

Uh oh!

gregmagolan commented Feb 16, 2020

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Mar 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gkalpak commented Feb 12, 2020 •

edited

Loading

gregmagolan Feb 13, 2020 •

edited

Loading