Repository navigation
Conversation
t.resolves(promise, message) only checks that the promise resolves; its second argument is the assertion message, not an expected value. These tests passed an expected value there, so the results were never compared: - test/lib/commands/access.js: all completion expectations - workspaces/libnpmaccess/test/index.js: getCollaborators, getPackages and getVisibility Await the result and compare it with t.strictSame instead.
The top-level `npm access` completion test expected ['list', 'get', 'set', 'grant', 'revoke'], but the command has always returned its subcommands alphabetically. This went unnoticed because the test used t.resolves, which never compared the value.
Once a subcommand was already typed (e.g. `npm access grant <TAB>`), completion() fell through without a return value and resolved to undefined. `npm completion` then threw on `undefined.length` and swallowed the error, so the empty result only worked by accident. Return [] so completion always resolves to an array, as the completion contract expects and as the existing test already assumed.
jeffkl
approved these changes
Oct 28, 2026
This file contains hidden or 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
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.
While working on #10184, I noticed that some tests were misusing tap's
t.resolves(). They seem to assume thatt.resolves(promise, 'value')checks that the promise resolves to'value', but the second parameter is actually the assertion message (its label).t.resolves()only checks that the promise resolves, not what it resolves to. I checked whether tap has ever worked the way these tests assume, but I couldn't see that it ever has.I fixed the tests that affected #10184, but figured there might be other examples, so I looked through the project and found the same issue in two places:
npm accesscommand tests (test/lib/commands/access.js)workspaces/libnpmaccess/test/index.js)This PR replaces those uses of
t.resolves()witht.strictSame(), which deep-compares the arrays and objects the promises resolve to. The libnpmaccess tests passed without changing any expected values.In the
npm accesscompletion test, two expectations turned out to be wrong once the values were actually compared:t.resolves().completion()returnedundefined.npm completion(wrap()inlib/commands/completion.js) then throws onundefined.length, inside atry/catchthat swallows the error. So users saw no difference, but the test makes the intended behaviour clear. I updatedcompletion()to return[], which matches the test and shouldn't change anything for users.