-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
[v9.x backport] test: fix require-deps-deprecation for installed deps #18077
Closed
Closed
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
test: fix require-deps-deprecation for installed deps
Test test-require-deps-deprecation.js was failing when user already had node installed with acorn in require.resolve range. Modified test to acknowledge the possibility and throw only if acorn is found in the deps directory. Also changed the deprecation test for v9.x: common.expectWarning was failing because the required deps now throw ReferenceErrors when not properly called internally in the right order. PR-URL: #17848 Fixes: #17148 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
- Loading branch information
commit c341f9308debb85507d84acde9851091b5c5549d
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
'use strict'; | ||
|
||
const common = require('../common'); | ||
const assert = require('assert'); | ||
// The v8 modules when imported leak globals. Disable global check. | ||
common.globalCheck = false; | ||
|
||
const deprecatedModules = [ | ||
'node-inspect/lib/_inspect', | ||
'node-inspect/lib/internal/inspect_client', | ||
'node-inspect/lib/internal/inspect_repl', | ||
'v8/tools/SourceMap', | ||
'v8/tools/codemap', | ||
'v8/tools/consarray', | ||
'v8/tools/csvparser', | ||
'v8/tools/logreader', | ||
'v8/tools/profile', | ||
'v8/tools/profile_view', | ||
'v8/tools/splaytree', | ||
'v8/tools/tickprocessor', | ||
'v8/tools/tickprocessor-driver' | ||
]; | ||
|
||
// Newly added deps that do not have a deprecation wrapper around it would | ||
// throw an error, but no warning would be emitted. | ||
const deps = [ | ||
'acorn/dist/acorn', | ||
'acorn/dist/walk' | ||
]; | ||
|
||
const throwingModules = () => { | ||
for (const m of deprecatedModules) { | ||
require(m); | ||
} | ||
}; | ||
|
||
assert.throws(throwingModules, ReferenceError); | ||
|
||
// Instead of checking require, check that resolve isn't pointing toward a | ||
// built-in module, as user might already have node installed with acorn in | ||
// require.resolve range. | ||
// Ref: https://github.com/nodejs/node/issues/17148 | ||
for (const m of deps) { | ||
let path; | ||
try { | ||
path = require.resolve(m); | ||
} catch (err) { | ||
assert.ok(err.toString().startsWith('Error: Cannot find module ')); | ||
continue; | ||
} | ||
assert.notStrictEqual(path, m); | ||
} |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem correct, it should be inside the loop or it will only test the first
require()
call that throws.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but that's one of the problems: I'm not sure every
require
throws.From what I've seen, some tools throw (typically
v8/tools/profile
), some other don't. Should I remove every module not throwing? Or is there a better way to test this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure either, let's ping the reviewers of the original PR.
cc: @TimothyGu @cjihrig @Trott @jasnell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checking for
ReferenceError
appears to only check thatrequire(m)
is the wrong way to include two of the modules in the list. The correct way to include (for example)'v8/tools/tickprocessor'
is notrequire('v8/tools/tickprocessor')
but rather something likeeval(process.binding('natives')['v8/tools/tickprocessor'])
when run with--expose-internals
.I think the test for
ReferenceError
should be removed entirely. The important thing to test is that a DeprecationWarning is being emitted, usingcommon.expectsWarning()
or whatever it is that's in thecommon
module for that. I'm going to change my review to Request Changes for that. Sorry that I missed that on first review!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, wait, I didn't approve it or even review this until now. It's only 8 hours old. Never mind. But still, the comment stands: Yeah, that should be changed. Sorry if I'm directly or indirectly responsible for that code in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Trott this is an attempt to backport #17848.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lpinca Thanks. I don't see a
ReferenceError
check in this file on the master branch so now I'm even more confused. :-DThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this should actually be backported.
EDIT: The test was added in #16392, which is semver-major. It was then updated in #15566, which is marked as don't land on v4, v6, v8, and v9. Then, there were only two tweaks to the test itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following @cjihrig 's comment I'm inclined to close my PR. It does not seem needed in v9 in fact.
However I wonder, is at least the
acorn
testing usefull? If no, I'll close this and ask for a 'don't land on v9' tag to be added on the original PR.