Skip to content

/ angular

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

Npm integration test manifest #35669

Closed
wants to merge 2 commits into from
Closed

Npm integration test manifest #35669

wants to merge 2 commits into from

Conversation

@gregmagolan
Copy link
Contributor

gregmagolan commented Feb 25, 2020 •
edited

Run /integration/ng_elements_schematics test with bazel using the MANIFEST file.

@filipesilva This manifest file should be useful in the CLI repo for your integration tests

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
@googlebot googlebot added the cla: yes label Feb 25, 2020
@pullapprove pullapprove bot requested a review from IgorMinar Feb 25, 2020
@gregmagolan gregmagolan requested review from josephperrott and removed request for IgorMinar Feb 25, 2020
@ngbot ngbot bot added this to the needsTriage milestone Feb 25, 2020
@gregmagolan gregmagolan force-pushed the gregmagolan:npm-integration-test-manifest branch from 49a5bcc to daaccea Feb 25, 2020
@josephperrott
josephperrott approved these changes Feb 25, 2020
View changes
Copy link
Member

josephperrott left a comment

LGTM
@gregmagolan gregmagolan force-pushed the gregmagolan:npm-integration-test-manifest branch from daaccea to 02177bb Feb 25, 2020
@gregmagolan
gregmagolan reviewed Feb 25, 2020
View changes
.circleci/config.yml
@@ -78,20 +78,6 @@ executors:
resource_class: << parameters.resource_class >>
working_directory: ~/ng

browsers-executor:

This comment has been minimized.

Sign in to view
Copy link
@gregmagolan

gregmagolan Feb 25, 2020

Author Contributor

Goodbye browsers container
@gregmagolan
build: add npm package manifest to npm_integration_test
Loading status checks…
7208b18
@gregmagolan gregmagolan force-pushed the gregmagolan:npm-integration-test-manifest branch 2 times, most recently from 3b3a073 to f7f8a7b Feb 25, 2020
@gregmagolan
test: run /integration/ng_elements_schematics test with bazel
Loading status checks…
5f3f7b7
@gregmagolan gregmagolan force-pushed the gregmagolan:npm-integration-test-manifest branch from f7f8a7b to 5f3f7b7 Feb 25, 2020
@filipesilva
filipesilva approved these changes Feb 26, 2020
View changes
@filipesilva
Copy link
Member

filipesilva commented Feb 26, 2020

Nice! Will have to take a look at the produced format later, but I have a pretty good idea from seeing how its consumed.
@gregmagolan
Copy link
Contributor Author

gregmagolan commented Feb 26, 2020

Nice! Will have to take a look at the produced format later, but I have a pretty good idea from seeing how its consumed.

Just a JSON file with a map of npm package name to absolute path on disk to the .tar.gz for that package. Archive either comes from a generated npm_package or ng_package rule or a pkg_tar of a root node_modules/foo folder so you can share dependencies from your root node_modules with your integration tests automatically so they are all on the same version and so that any post-install steps that can be cached will be.

Puppeteer, for example, doesn't need to download Chrome again for the integrations tests since the root /node_modules/puppeteer is passed to the integration tests via .tar.gz. Integration tests also do not need to run webdriver-manager update as **/webdriver-manager is resolved to the archive of /node_modules/webdriver-manager.
@gregmagolan
Copy link
Contributor Author

gregmagolan commented Feb 26, 2020

Hmmm. Thinking out loud. If the filename of the .tar.gz that is used could include a hash of the contents then would we still need the .yarn-local-cache work-around? If the contents of the package changed then the file:/path/to/package-<hash>.tar.gz reference would as well.
mhevery added a commit that referenced this pull request Feb 26, 2020
@gregmagolan @mhevery
build: add npm package manifest to npm_integration_test (#35669)
9ff778a 
PR Close #35669
mhevery added a commit that referenced this pull request Feb 26, 2020
@gregmagolan @mhevery
test: run /integration/ng_elements_schematics test with bazel (#35669)
Loading status checks…
788d5d7 
PR Close #35669
@mhevery mhevery closed this in 1f3dd00 Feb 26, 2020
mhevery added a commit that referenced this pull request Feb 26, 2020
@gregmagolan @mhevery
test: run /integration/ng_elements_schematics test with bazel (#35669)
Loading status checks…
184a849 
PR Close #35669
@filipesilva
Copy link
Member

filipesilva commented Feb 27, 2020

@gregmagolan nifty stuff, thanks for doing this!

I think yarn only checks package and version name when determining if a cached version should be used. I think @gkalpak might have debugged this in depth in the past.
@angular-automatic-lock-bot
Copy link

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

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 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@filipesilva filipesilva

@josephperrott josephperrott

Assignees
No one assigned
Labels
PR action: merge PR target: master & patch cla: yes comp: build & ci
Projects
None yet
Milestone
  needsTriage
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@gregmagolan @filipesilva @josephperrott @googlebot
You can’t perform that action at this time.