-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
### 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
force-pushed
the
lk/remove-fake-mock-npm
branch
from
January 1, 2023 22:23
2979fb1
to
917d70b
Compare
lukekarrys
force-pushed
the
lk/remove-fake-mock-npm
branch
2 times, most recently
from
January 1, 2023 22:50
4d795cc
to
52a0168
Compare
no statistically significant performance changes detected timing results
|
lukekarrys
changed the title
Refactor and remove
Refactor and remove fakeMockNpm
Jan 1, 2023
fakeMockNpm
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
force-pushed
the
lk/remove-fake-mock-npm
branch
from
January 1, 2023 23:00
52a0168
to
547f0c0
Compare
Seeing as this refactor clocked in at It also includes a fix for nodejs/node#45881 which would be nice to get in |
5 tasks
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 settinglocation=project
which affected all commands such asconfig set
saving to the wrong location.
Some notable changes from this refactor include:
execWorkspaces
no longer being passed the raw workspace filters andinstead requiring
this.setWorkspaces()
to be called which wasalready being done in most commands
static workspaces = [true|false]
on all commands to indicateworkspaces support. This is used in docs generation and whether to
throw when
execWorkspaces
is called or not.Drop
fakeMockNpm
and refactormockNpm
This refactor also drops
fakeMockNpm
in favor ofrealMockNpm
(nowjust
mockNpm
), and adds new features tomockNpm
.Some new features of
mockNpm
:npm.load()
. Thisis the most important change as it more closely resembles real usage.
process.exitCode
node_modules
in correct testdir based on platformprocess.chdir
automatically toprefix
and setsglobalPrefix
to thespecified 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 properlynpm init -w path/ws
was writng\\
separators to package.json on windowsnpm view
now usesnpm.output
instead ofconsole.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
which
confighelp
to use@npmcli/promise-spawn
mockNpm