Skip to content

build: npm_integration_test && angular_integration_test #33927

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

Conversation

gregmagolan
Copy link
Contributor

@gregmagolan gregmagolan commented Nov 20, 2019

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

@IgorMinar IgorMinar self-requested a review November 20, 2019 09:07
@IgorMinar
Copy link
Contributor

This PR looks very promising!!

I ran across some issues however... so it still needs polish (as you already mentioned on slack)...

yarn bazel test //integration:cli_hello_world_test --test_output=streamed

results in


[test_runner.js] running test command '/b/f/w/bazel-out/k8-fastbuild/bin/integration/cli_hello_world_test.sh.runfiles/nodejs_darwin_amd64/bin/yarn install' in /tmp/tmp-194EFNZxTaipL8
/b/f/w/bazel-out/k8-fastbuild/bin/integration/cli_hello_world_test.sh.runfiles/nodejs_darwin_amd64/bin/yarn: line 18: /b/f/w/bazel-out/k8-fastbuild/bin/integration/cli_hello_world_test.sh.runfiles/nodejs_darwin_amd64/bin/node: No such file or directory
[test_runner.js] 0 of 2 test commands successful
[test_runner.js] test command 1 failed with status code 1
[test_runner.js] to run test in debug mode:

    bazel run //integration:cli_hello_world_test.debug

on mac

I also tried bazel run //integration:cli_hello_world_test.debug which fails with:

** Angular Live Development Server is listening on localhost:4200, open your browser on http://localhost:4200/ **
: Compiled successfully.
[09:23:55] I/launcher - Running 1 instances of WebDriver
[09:23:55] I/direct - Using ChromeDriver directly...
[09:23:57] E/launcher - session not created: This version of ChromeDriver only supports Chrome version 75
  (Driver info: chromedriver=75.0.3770.90 (a6dcaf7e3ec6f70a194cc25e8149475c6590e025-refs/branch-heads/3770@{#1003}),platform=Mac OS X 10.14.6 x86_64)
[09:23:57] E/launcher - SessionNotCreatedError: session not created: This version of ChromeDriver only supports Chrome version 75

which is different from the version I have installed locally (78.0.3904.97) - interestingly this hasn't been a problem I experienced before which is odd. I tried to manually run the webdriver-manager command to install version 78.0.3904.105 and then I got a bit further.

next I did get a build error:

This version of CLI is only compatible with Angular versions ^9.0.0-beta || >=9.0.0 <10.0.0,
but Angular version 0.0.0 was found instead.

This is due to: https://github.com/angular/angular-cli/blob/c7135fae357ffe16bd420b59b80b002632806a45/packages/angular_devkit/build_angular/src/utils/version.ts#L55-L78

I tried to use --config=release as well but that didn't work for me and resulted in the same error.

In any case, build stamping is expensive because it invalidates most caches.

This check comes from: https://github.com/angular/angular-cli/blob/c7135fae357ffe16bd420b59b80b002632806a45/packages/angular_devkit/build_angular/src/utils/version.ts#L55-L58

I worked around this in the past by monkey-patching @angular/cli in node_modules and setting its version to 0.0.0 which disables the check (see cli code link above), but it might be even better to update that CLI version check to be angularCliPkgJson['version'] === '0.0.0' || angularPkgJson['version'] === '0.0.0'. This might be worth requesting from the CLI team or sending them a PR.

One more issue I see lurking is yarn caching issue of file:// dependencies (yarnpkg/yarn#2165) - I believe that I don't see my updates to packages/core/src reflected after rerunning the .debug command and I wonder if that yarn issue is the culprit.

Lastly, I wonder if running yarn test automatically as part of the .debug workflow is ideal. Or maybe .debug should do that but there also should be a .install workflow that just sets up the project with locally built packages built and installed and leaves it up to me to run yarn build or yarn test.

Sorry, I vomited a lot of stuff into this commit :-) I'm just a bit too excited about the prospect of this actually working and us being able to cache the test results of these integration tests, and more easily rerun these tests locally and be able to iterate on core packages while observing the impact of those changes on these integration apps/tests.

@gregmagolan
Copy link
Contributor Author

gregmagolan commented Nov 20, 2019

Thanks for the taking a look @IgorMinar .

  • node node found issue came up in CI as well so will fix that today.

  • The config:release issue I think we should fix in the CLI so that it allows version "0.0.0" otherwise we'ld have to have --workspace_status_command="node ./tools/bazel_stamp_vars.js" on for the build which is not desirable talking to @josephperrott as building with "0.0.0" in general prevents accidental publishes. Note: when you add --config:release after the build is complete once it won't re-run the build as changes to bazel-out/volatile-status.txt does not invalidate the build cache.

  • Chromedriver issue is interesting. It seems like just running webdriver-manager update --gecko=false --standalone=false installs chromedriver_75.0.3770.140 by default. The test postinstall has webdriver-manager update --gecko=false --standalone=false $CI_CHROMEDRIVER_VERSION_ARG and $CI_CHROMEDRIVER_VERSION_ARG is not set. I also had to run ``webdriver-manager update --gecko=false --standalone=false --versions.chrome 78.0.3904.105` to get the correct version that works with my local chrome.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

this PR is getting insanely complex :-(

to be honest, I can't really follow what's going on here unless I set aside half day to study all the different parts and how they interact.

I'm really concerned that we won't be able to maintain this stuff. Aren't there ways to simplify the approach?

@gregmagolan
Copy link
Contributor Author

gregmagolan commented Nov 22, 2019

I was thinking I would upstream the npm_integration_test to rules_nodejs as it will be useful for any project that generates npm_packages and wants to integration test against the generated artifacts. I don't think it would make sense for the angular team to maintain this it is a general solution for integration testing against npm_package artifacts. The angular repo .bzl portion would just be the angular_integration_test macro which sets some defaults.

@gregmagolan gregmagolan force-pushed the npm_integration_test branch 2 times, most recently from 0d28dfa to 4f24dd6 Compare November 22, 2019 18:35
mgechev pushed a commit to angular/angular-cli that referenced this pull request Nov 22, 2019
…0.0.0

Allow version "0.0.0" for integration testing in the angular/angular repository with the generated development @angular/core npm package which is versioned "0.0.0".
This is a pre-req for angular/angular#33927 which runs integration tests against the bazel generated npm packages.
mgechev pushed a commit to angular/angular-cli that referenced this pull request Nov 22, 2019
…0.0.0

Allow version "0.0.0" for integration testing in the angular/angular repository with the generated development @angular/core npm package which is versioned "0.0.0".
This is a pre-req for angular/angular#33927 which runs integration tests against the bazel generated npm packages.
@gregmagolan gregmagolan force-pushed the npm_integration_test branch 4 times, most recently from ab20f46 to ab4640a Compare November 24, 2019 20:54
@gregmagolan
Copy link
Contributor Author

gregmagolan commented Nov 24, 2019

Alrighty. Based on your comments @IgorMinar I've grossly simplified the rule. It is now a macro that generates a .js config file & then calls a nodejs test with that config file as the first argument.

Can't land this yet as it is waiting on:

  1. The next release of angular-cli to make it to angular which includes refactor(@angular-devkit/build-angular): allow @angular/core version 0.0.0 angular-cli#16267 to be able to remove the CLI patch
  2. build: update to nodejs rules 0.41.0 #33996 to be able to remove the rules_nodejs patch

Next step is to expand this too all of the integration tests (except for the bazel-in-bazel tests integration/bazel and integration/bazel-schematics) and remove those tests from CI and from the integration.sh script.

Not sure if I should land this here or upstream the npm_integration_test portion and land this PR after that makes it into the next rules_nodejs release.

@gregmagolan gregmagolan added the area: build & ci Related the build and CI infrastructure of the project label Nov 24, 2019
@ngbot ngbot bot added this to the needsTriage milestone Nov 24, 2019
@gregmagolan gregmagolan added area: bazel Issues related to the published `@angular/bazel` build rules target: patch This PR is targeted for the next patch release labels Nov 24, 2019
@gregmagolan gregmagolan marked this pull request as ready for review November 24, 2019 21:54
Move bazel-in-bazel them to test job & increase it is 2xlarge+. test_integration_bazel is removed. Overall CI credit usage is reduced.

test: include ng_elements_schematics in legacy integration tests temporarily

This test was recently added and use a new pattern that doesn't work with npm_integration_test out of the box. It needs some refactoring to work. Left a TODO for this
@gregmagolan gregmagolan added the action: merge The PR is ready for merge by the caretaker label Feb 24, 2020
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGTM!!! We can follow up on this with further improvements in additional PRs. Thanks @gregmagolan !!!

@mhevery mhevery closed this in dde68ff Feb 24, 2020
mhevery pushed a commit that referenced this pull request Feb 24, 2020
For now until RBE actions can access network for `yarn install`

PR Close #33927
mhevery pushed a commit that referenced this pull request Feb 24, 2020
Install java runtime which is required by some integration tests such as `//integration:hello_world__closure_test`, `//integration:i18n_test` and `//integration:ng_elements_test` to run the closure compiler.

PR Close #33927
mhevery pushed a commit that referenced this pull request Feb 24, 2020
Now that large integration tests are running locally in parallel (they can't run on RBE yet as they require network access for yarn install), this test is running out of memory consistently with the xlarge machine

PR Close #33927
mhevery pushed a commit that referenced this pull request Feb 24, 2020
…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
mhevery pushed a commit that referenced this pull request Feb 24, 2020
mhevery pushed a commit that referenced this pull request Feb 24, 2020
…3927)

Move bazel-in-bazel them to test job & increase it is 2xlarge+. test_integration_bazel is removed. Overall CI credit usage is reduced.

test: include ng_elements_schematics in legacy integration tests temporarily

This test was recently added and use a new pattern that doesn't work with npm_integration_test out of the box. It needs some refactoring to work. Left a TODO for this

PR Close #33927
mhevery pushed a commit that referenced this pull request Feb 24, 2020
mhevery pushed a commit that referenced this pull request Feb 24, 2020
mhevery pushed a commit that referenced this pull request Feb 24, 2020
* it's tricky to get out of the runfiles tree with `bazel test` as `BUILD_WORKSPACE_DIRECTORY` is not set but I employed a trick to read the `DO_NOT_BUILD_HERE` file that is one level up from `execroot` and that contains the workspace directory. This is experimental and if `bazel test //:test.debug` fails than `bazel run` is still guaranteed to work as  `BUILD_WORKSPACE_DIRECTORY` will be set in that context

* test //integration:bazel_test and //integration:bazel-schematics_test exclusively

* run "exclusive" and "manual" bazel-in-bazel integration tests in their own CI job as they take 8m+ to execute

```
//integration:bazel-schematics_test                                      PASSED in 317.2s
//integration:bazel_test                                                 PASSED in 167.8s
```

* Skip all integration tests that are now handled by angular_integration_test except the tests that are tracked for payload size; these are:
- cli-hello-world*
- hello_world__closure

* add & pin @babel deps as newer versions of babel break //packages/localize/src/tools/test:test

@babel/core dep had to be pinned to 7.6.4 or else //packages/localize/src/tools/test:test failed. Also //packages/localize uses @babel/generator, @babel/template, @babel/traverse & @babel/types so these deps were added to package.json as they were not being hoisted anymore from @babel/core transitive.

NB: integration/hello_world__systemjs_umd test must run with systemjs 0.20.0
NB: systemjs must be at 0.18.10 for legacy saucelabs job to pass
NB: With Bazel 2.0, the glob for the files to test `"integration/bazel/**"` is empty if integation/bazel is in .bazelignore. This glob worked under these conditions with 1.1.0. I did not bother testing with 1.2.x as not having integration/bazel in .bazelignore is correct.

PR Close #33927
mhevery pushed a commit that referenced this pull request Feb 24, 2020
For now until RBE actions can access network for `yarn install`

PR Close #33927
mhevery pushed a commit that referenced this pull request Feb 24, 2020
Install java runtime which is required by some integration tests such as `//integration:hello_world__closure_test`, `//integration:i18n_test` and `//integration:ng_elements_test` to run the closure compiler.

PR Close #33927
mhevery pushed a commit that referenced this pull request Feb 24, 2020
Now that large integration tests are running locally in parallel (they can't run on RBE yet as they require network access for yarn install), this test is running out of memory consistently with the xlarge machine

PR Close #33927
mhevery pushed a commit that referenced this pull request Feb 24, 2020
…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
mhevery pushed a commit that referenced this pull request Feb 24, 2020
mhevery pushed a commit that referenced this pull request Feb 24, 2020
…3927)

Move bazel-in-bazel them to test job & increase it is 2xlarge+. test_integration_bazel is removed. Overall CI credit usage is reduced.

test: include ng_elements_schematics in legacy integration tests temporarily

This test was recently added and use a new pattern that doesn't work with npm_integration_test out of the box. It needs some refactoring to work. Left a TODO for this

PR Close #33927
mhevery pushed a commit that referenced this pull request Feb 24, 2020
mhevery pushed a commit that referenced this pull request Feb 24, 2020
@angular-automatic-lock-bot
Copy link

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 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: bazel Issues related to the published `@angular/bazel` build rules area: build & ci Related the build and CI infrastructure of the project cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants