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

Refactor and remove fakeMockNpm #6014

Merged
merged 10 commits into from Jan 1, 2023
Merged

Refactor and remove fakeMockNpm #6014

merged 10 commits into from Jan 1, 2023

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Jan 1, 2023

Refactor Config Reading During exec

This removes the previous workarounds that were required due to tests
setting configs that were not available in the constructor of the
commands.

This also made the fix for nodejs/node#45881
easier since the config checks for workspaces can now all be made in the
command. The fix was to move the package.json detection to
@npmcli/config and use that boolean instead of setting
location=project which affected all commands such as config set
saving to the wrong location.

Some notable changes from this refactor include:

  • execWorkspaces no longer being passed the raw workspace filters and
    instead requiring this.setWorkspaces() to be called which was
    already being done in most commands
  • static workspaces = [true|false] on all commands to indicate
    workspaces support. This is used in docs generation and whether to
    throw when execWorkspaces is called or not.

Drop fakeMockNpm and refactor mockNpm

This refactor also drops fakeMockNpm in favor of realMockNpm (now
just mockNpm), and adds new features to mockNpm.

Some new features of mockNpm:

  • sets all configs via argv so they are parsed before npm.load(). This
    is the most important change as it more closely resembles real usage.
  • automatically resets process.exitCode
  • automatically puts global node_modules in correct testdir based on platform
  • more helpful error messages when used in unsupported ways
  • ability to preload a command for execution
  • calls process.chdir automatically to prefix and sets globalPrefix to the
    specified global testdir

Misc Fixes

During the test refactor I uncovered some bugs that were also fixed or features
that were made easier to implement

  • npm fund --which wasn't validated properly
  • npm init -w path/ws was writng \\ separators to package.json on windows
  • npm view now uses npm.output instead of console.log

Linting

This also removes some of the one off linting rules that were set in the
past to reduce churn and fixes all remaining errors.

Commits

  • fix: evaluate configs in command class
  • fix: generate workspace support for docs pages
  • fix(fund): correctly parse and use which config
  • fix: refactor help to use @npmcli/promise-spawn
  • fix(init): write package.json workspaces paths with / separators
  • fix(view): convert command to use output instead of console
  • chore: convert ls test to mockNpm
  • chore: convert diff test to mock npm and registry
  • chore: convert all test to mockNpm
  • chore: use chdir for mockNpm

 ### Refactor Config Reading During `exec`

This removes the previous workarounds that were required due to tests
setting configs that were not available in the constructor of the
commands.

This also made the fix for nodejs/node#45881
easier since the config checks for workspaces can now all be made in the
command. The fix was to move the package.json detection to
`@npmcli/config` and use that boolean instead of setting
`location=project` which affected all commands such as `config set`
saving to the wrong location.

Some notable changes from this refactor include:
-  `execWorkspaces` no longer being passed the raw workspace filters and
   instead requiring `this.setWorkspaces()` to be called which was
   already being done in most commands
- `static workspaces = [true|false]` on all commands to indicate
  workspaces support. This is used in docs generation and whether to
  throw when `execWorkspaces` is called or not.

 ### Drop `fakeMockNpm` and refactor `mockNpm`

This refactor also drops `fakeMockNpm` in favor of `realMockNpm` (now
just `mockNpm`), and adds new features to `mockNpm`.

Some new features of `mockNpm`:

- sets all configs via argv so they are parsed before `npm.load()`. This
  is the most important change as it more closely resembles real usage.
- automatically resets `process.exitCode`
- automatically puts global `node_modules` in correct testdir based on platform
- more helpful error messages when used in unsupported ways
- ability to preload a command for execution
- sets `cwd` automatically to `prefix` and sets `globalPrefix` to the
  specified testdir

Note that this commit does not include the actual test changes, which
are included in the following commits for readability reasons.

 ### Linting

This also removes some of the one off linting rules that were set in the
past to reduce churn and fixes all remaining errors.
@lukekarrys lukekarrys requested a review from a team as a code owner January 1, 2023 22:22
@lukekarrys lukekarrys changed the title lk/remove fake mock npm Refactor and remove fakeMockNpm Jan 1, 2023
@lukekarrys lukekarrys force-pushed the lk/remove-fake-mock-npm branch 2 times, most recently from 4d795cc to 52a0168 Compare January 1, 2023 22:50
@npm-cli-bot
Copy link
Collaborator

npm-cli-bot commented Jan 1, 2023

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 64.374 ±1.72 27.540 ±0.08 33.623 ±11.07 29.234 ±1.48 4.426 ±0.05 4.798 ±0.29 3.644 ±0.01 17.564 ±0.15 3.602 ±0.10 5.420 ±0.17
#6014 63.720 ±6.17 27.238 ±0.16 34.394 ±13.34 28.792 ±0.26 4.382 ±0.10 4.669 ±0.01 3.481 ±0.09 17.847 ±0.24 3.446 ±0.04 5.251 ±0.50
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 37.827 ±5.03 20.822 ±0.26 18.922 ±0.16 20.495 ±0.26 3.942 ±0.06 4.001 ±0.08 3.606 ±0.04 13.305 ±0.17 3.470 ±0.09 4.766 ±0.05
#6014 37.752 ±2.68 20.549 ±0.03 19.083 ±0.12 20.661 ±0.57 3.941 ±0.09 4.015 ±0.04 3.465 ±0.02 12.922 ±0.06 3.314 ±0.03 4.785 ±0.13

@lukekarrys lukekarrys changed the title Refactor and remove fakeMockNpm Refactor and remove fakeMockNpm Jan 1, 2023
Previously the `which` param in `npm fund` was validated incorrectly
leading to `EFUNDNUMBER` errors when used. This fixes that and does a
better job detecting when `which` is pointing to an incorrect array
bounds in the returned funding array.
This also refactors the test to use `mockNpm`
This also refactors the test to use `mockNpm`
This also uses more consistent snapshot cleaning where possible.

Note that this is an iterative step for our tests and does not implement
`MockRegistry` for them, so many of them still mock important
functionality that should be tested. This refactor to `mockNpm` should
make it easier to adopt `MockRegistry` as we see fit.
@lukekarrys
Copy link
Contributor Author

lukekarrys commented Jan 1, 2023

Seeing as this refactor clocked in at +11k, -12k lines and is primarily test related, I'm going to merge this without review, since it would be very cumbersome to review at this point.

It also includes a fix for nodejs/node#45881 which would be nice to get in 9.2.1 for inclusion in the Node release slated for 2023-01-03.

@lukekarrys lukekarrys merged commit 122938c into latest Jan 1, 2023
@lukekarrys lukekarrys deleted the lk/remove-fake-mock-npm branch January 1, 2023 23:14
@github-actions github-actions bot mentioned this pull request Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants