-
Notifications
You must be signed in to change notification settings - Fork 26.2k
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
Conversation
This PR looks very promising!! I ran across some issues however... so it still needs polish (as you already mentioned on slack)...
results in
on mac I also tried
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:
I tried to use 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 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 Lastly, I wonder if running 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. |
Thanks for the taking a look @IgorMinar .
|
6a4c880
to
0cbc0ce
Compare
e086d2b
to
ee72db2
Compare
There was a problem hiding this 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?
I was thinking I would upstream the |
0d28dfa
to
4f24dd6
Compare
…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.
…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.
ab20f46
to
ab4640a
Compare
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:
Next step is to expand this too all of the integration tests (except for the bazel-in-bazel tests 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. |
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
303e791
to
6a46bac
Compare
6a46bac
to
9ec7ec8
Compare
There was a problem hiding this 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 !!!
For now until RBE actions can access network for `yarn install` PR Close #33927
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
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
…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
* 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
For now until RBE actions can access network for `yarn install` PR Close #33927
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
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
…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
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. |
PR 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