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

fix(tests): move more tests to use real npm #3463

Merged
merged 1 commit into from Jul 29, 2021

Conversation

wraithgar
Copy link
Member

This moves a handful of the smaller tests to using the new npm mock that
uses the real actual npm object. It also extends the testing surface
area of a few tests back down into the actual process.spawn that
results, instead of anything internal to the code.

Some dead code in lib/test.js was found during this, as well as an
instance of a module throwing a string instead of an error object.

@wraithgar wraithgar requested a review from a team as a code owner June 24, 2021 17:12
@wraithgar wraithgar added release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes labels Jun 24, 2021

exec (args, cb) {
super.exec(args, er => {
if (er && er.code === 'ELIFECYCLE') {
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing in any of the cli code or its dependencies sets this, and the exit handler suppresses the callback error during any command that shells out anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is clearly dead code.

package.json Outdated
@@ -126,6 +126,7 @@
"@npmcli/arborist",
"@npmcli/ci-detect",
"@npmcli/config",
"@npmcli/package-json",
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the bundle and gitignore didn't get ran in the packge-json PR.

@wraithgar
Copy link
Member Author

wraithgar commented Jun 24, 2021

Tests are failing because npm check-coverage is only ran in one environment and it's failing there now.

As far as I can tell it should ALWAYS have been failing.

ETA: it's failing in release-next and latest for me too. This is not this PR's problem.

@wraithgar wraithgar marked this pull request as draft July 2, 2021 17:46
@ruyadorno ruyadorno removed the release: next These items should be addressed in the next release label Jul 15, 2021
@wraithgar wraithgar marked this pull request as ready for review July 28, 2021 16:30
@wraithgar
Copy link
Member Author

Looks like we crossed back over the coverage threshold in the meantime so we can just go forward w/ this w/o having to do the much larger "fix all the coverage" task

Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

this all looks good to me!

This moves a handful of the smaller tests to using the new npm mock that
uses the real actual npm object.  It also extends the testing surface
area of a few tests back down into the actual `process.spawn` that
results, instead of anything internal to the code.

Some dead code in `lib/test.js` was found during this, as well as an
instance of a module throwing a string instead of an error object.

PR-URL: #3463
Credit: @wraithgar
Close: #3463
Reviewed-by: @nlf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants