Skip to content
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

test: remove use of global npm cache and config #23115

Merged
merged 1 commit into from May 24, 2022

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented May 11, 2022

Removing the use of globally installed npm and npm packages. Instead npm+yarn within the test-runner use npm+yarn from package.json (not globals). Tests no longer depend on any --global and use npm+ng installed locally within the temporary test directories.

This might be the last step before being able to run multiple tests concurrently (#23114 was the other big one).

This might be the last one before getting the e2e tests working in bazel (#23074)

EDIT: this has changed to keep globals but use a isolated global cache per invocation, see comment below

Original attempt removing use of --global: 02f7039

@jbedard jbedard force-pushed the no-global-npm branch 9 times, most recently from 979097e to 3d4d936 Compare May 12, 2022 23:59
@jbedard jbedard marked this pull request as ready for review May 13, 2022 01:03
.circleci/config.yml Outdated Show resolved Hide resolved
@jbedard
Copy link
Contributor Author

jbedard commented May 19, 2022

Note this started failing after the recent npm changes. I'll try getting it passing again but let me know if there's any feedback on what's here so far...

package.json Outdated Show resolved Hide resolved
tests/legacy-cli/e2e/setup/200-install-cli.ts Outdated Show resolved Hide resolved
tests/legacy-cli/e2e/setup/200-install-cli.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@jbedard jbedard force-pushed the no-global-npm branch 2 times, most recently from b9effb5 to 4dadb25 Compare May 20, 2022 02:08
@jbedard jbedard changed the title test: remove use of global npm installs test: remove use of global npm cache and config May 20, 2022
@jbedard jbedard force-pushed the no-global-npm branch 3 times, most recently from 07ebfc2 to faa19bb Compare May 20, 2022 18:15
@jbedard jbedard force-pushed the no-global-npm branch 3 times, most recently from 25bd8da to f41341f Compare May 20, 2022 20:03
@jbedard jbedard requested a review from clydin May 20, 2022 20:23
@jbedard
Copy link
Contributor Author

jbedard commented May 20, 2022

Ok I've updated this PR to significantly less changes then previously...

  • each test run uses an isolated folder for the global installations, that global npm bin dir is added to the PATH
  • each test run uses only an empty npmrc and prevents the use of ~/.npmrc and the global one
  • those are both configured via environment variables, those variables must be propagated to child node processes (e2e tests, npm installs etc)

This no longer removes --global or changes how npm/yarn versions are determined like it original did.

This does NOT prevent the use of the shared global npm database (~/.npm/_cacache/ configured via NPM_CONFIG_CACHE env var) unlike the npm global modules which are isolated. So despite the installed global modules being isolated they shouldn't have to be re-downloaded if the global database is populated. This might change once bazel is used because the bazel test process doesn't have write permission to that global database.

@jbedard jbedard force-pushed the no-global-npm branch 4 times, most recently from 8d70785 to 289ab02 Compare May 20, 2022 22:21
Copy link
Member

@clydin clydin left a comment

Choose a reason for hiding this comment

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

LGTM

@clydin clydin added the target: minor This PR is targeted for the next minor release label May 24, 2022
@clydin clydin removed the request for review from alan-agius4 May 24, 2022 16:49
@clydin clydin added the action: merge The PR is ready for merge by the caretaker label May 24, 2022
@dgp1130 dgp1130 merged commit 9cb4eaf into angular:main May 24, 2022
@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 Jun 24, 2022
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 target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants