[v9.x backport] test: fix require-deps-deprecation for installed deps #18077

Tiriel · 2018-01-10T12:54:10Z

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

Original PR: #17848

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

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: nodejs#17848 Fixes: nodejs#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>

Tiriel · 2018-01-10T12:54:32Z

I don't know if this is the correct way to adapt this test, but I figured it was the best.

For the explanation:
The initial commit could not be backported on v9.x . After some investigation, the problem did not come from the require test modification, but from the deprecation warning expectations. From what I saw, these expectations where failing because require is now throwing ReferenceError when the internal deps are not properly called (ie internally to Node, not in a loop, and in the proper order).
So, this solution appeared the best to me. But I'll gladly amend in any way, or even remove this PR if this cannot be backported as it is now.

(Obviously though, I can't run the CI job as explained in the backporting guide as I don't have permissions)

lpinca · 2018-01-10T17:00:39Z

test/parallel/test-require-deps-deprecation.js

+  }
+};
+
+assert.throws(throwingModules, ReferenceError);


This doesn't seem correct, it should be inside the loop or it will only test the first require() call that throws.

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?

I'm not sure either, let's ping the reviewers of the original PR.
cc: @TimothyGu @cjihrig @Trott @jasnell

The checking for ReferenceError appears to only check that require(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 not require('v8/tools/tickprocessor') but rather something like eval(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, using common.expectsWarning() or whatever it is that's in the common module for that. I'm going to change my review to Request Changes for that. Sorry that I missed that on first review!

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.

@Trott this is an attempt to backport #17848.

@lpinca Thanks. I don't see a ReferenceError check in this file on the master branch so now I'm even more confused. :-D

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.

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.

Trott · 2018-01-10T20:30:23Z

test/parallel/test-require-deps-deprecation.js

+  }
+};
+
+assert.throws(throwingModules, ReferenceError);


The checking for ReferenceError appears to only check that require(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 not require('v8/tools/tickprocessor') but rather something like eval(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, using common.expectsWarning() or whatever it is that's in the common module for that. I'm going to change my review to Request Changes for that. Sorry that I missed that on first review!

Tiriel · 2018-01-10T21:13:06Z

@Trott That’s why I changed the test in fact: no warning is emitted, because an error is thrown as they are not properly required.

The test was failing because no warning was emitted, so I removed the try-catch and got errors saying that CodeMap wasn’t defined in profile.js .

It seems this test is pointless if these deeps cannot be required without throwing. Or is there some other way?

Trott · 2018-01-10T21:40:27Z

Ah, yeah, you'll need to remove any modules that aren't deprecated in 9.x. If that means removing all of them, then yeah, we don't need the test in 9.x. :-D

Tiriel · 2018-01-11T13:47:57Z

Made some more tests and I can confirm, the test on acorn still seems relevant, but I don't know if it's worth keeping it.

As for the other tests, no v8/tool seem to be emitting warnings on v9.x so I just dropped them.

Fixes: nodejs#17148

requested change has been made

Tiriel · 2018-01-15T14:14:23Z

Just a small up.
Does anyone thinks this can/should be merged, or do I drop this PR?

Trott · 2018-01-16T23:05:56Z

@nodejs/testing

BridgeAR · 2018-02-13T05:33:12Z

Ping @nodejs/lts

jasnell · 2018-02-13T23:39:04Z

Why is this targeting v9.x instead of master?
And if it is v9.x, then pinging @nodejs/lts is not necessary since 9.x is not an LTS branch.

richardlau · 2018-02-14T00:57:44Z

Why is this targeting v9.x instead of master?

This is a requested backport (#17848 (comment)) of a PR that has already landed on master. I've amended the PR title so that it is clearer.

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. Bacport-PR-URL: #18077 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>

MylesBorins · 2018-02-20T21:30:44Z

Landed in b5752ee

Tiriel · 2018-02-20T21:40:16Z

Thanks!

nodejs-github-bot added test Issues and PRs related to the tests. v9.x labels Jan 10, 2018

lpinca reviewed Jan 10, 2018

View reviewed changes

Trott previously requested changes Jan 10, 2018

View reviewed changes

test: removed deprecation tests

e93767f

Fixes: nodejs#17148

richardlau changed the title ~~test: fix require-deps-deprecation for installed deps~~ [v9.x backport] test: fix require-deps-deprecation for installed deps Feb 14, 2018

MylesBorins closed this Feb 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v9.x backport] test: fix require-deps-deprecation for installed deps #18077

[v9.x backport] test: fix require-deps-deprecation for installed deps #18077

Tiriel commented Jan 10, 2018

Tiriel commented Jan 10, 2018 •

edited

Loading

lpinca Jan 10, 2018 •

edited

Loading

Tiriel Jan 10, 2018 •

edited

Loading

lpinca Jan 10, 2018

Trott Jan 10, 2018

Trott Jan 10, 2018

lpinca Jan 10, 2018

Trott Jan 10, 2018

cjihrig Jan 10, 2018 •

edited

Loading

Tiriel Jan 11, 2018

Trott Jan 10, 2018

Tiriel commented Jan 10, 2018

Trott commented Jan 10, 2018 •

edited

Loading

Tiriel commented Jan 11, 2018

Tiriel commented Jan 15, 2018

Trott commented Jan 16, 2018

BridgeAR commented Feb 13, 2018

jasnell commented Feb 13, 2018

richardlau commented Feb 14, 2018

MylesBorins commented Feb 20, 2018

Tiriel commented Feb 20, 2018

[v9.x backport] test: fix require-deps-deprecation for installed deps #18077

[v9.x backport] test: fix require-deps-deprecation for installed deps #18077

Conversation

Tiriel commented Jan 10, 2018

Checklist

Affected core subsystem(s)

Tiriel commented Jan 10, 2018 • edited Loading

lpinca Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Tiriel Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

lpinca Jan 10, 2018

Choose a reason for hiding this comment

Trott Jan 10, 2018

Choose a reason for hiding this comment

Trott Jan 10, 2018

Choose a reason for hiding this comment

lpinca Jan 10, 2018

Choose a reason for hiding this comment

Trott Jan 10, 2018

Choose a reason for hiding this comment

cjihrig Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Tiriel Jan 11, 2018

Choose a reason for hiding this comment

Trott Jan 10, 2018

Choose a reason for hiding this comment

Tiriel commented Jan 10, 2018

Trott commented Jan 10, 2018 • edited Loading

Tiriel commented Jan 11, 2018

Tiriel commented Jan 15, 2018

Trott commented Jan 16, 2018

BridgeAR commented Feb 13, 2018

jasnell commented Feb 13, 2018

richardlau commented Feb 14, 2018

MylesBorins commented Feb 20, 2018

Tiriel commented Feb 20, 2018

Tiriel commented Jan 10, 2018 •

edited

Loading

lpinca Jan 10, 2018 •

edited

Loading

Tiriel Jan 10, 2018 •

edited

Loading

cjihrig Jan 10, 2018 •

edited

Loading

Trott commented Jan 10, 2018 •

edited

Loading