tests: fix test-require-deps-deprecation for already installed deps#17848

Tiriel · 2017-12-24T08:15:01Z

Merry christmas!

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.

Fixes: #17148
Refs: #17148 (comment)

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

TimothyGu · 2017-12-24T08:22:21Z

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

This wouldn't be necessary.

Do you mean the comment, or the 'foo' dep too?

The “foo” dep… and as a result the comment too.

TimothyGu · 2017-12-24T08:22:59Z

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

nit: I don't think we ever use the $e convention. err is fine.

Damn, sorry, too much PHP lately I think...

TimothyGu · 2017-12-24T08:25:27Z

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

To avoid comparing things to the empty string this could be refactored as:
letpath;try{path=require.resolve(m);}catch(err){continue;}assert.notStrictEqual(path,m);
Nice trick with require.resolve though :)

Thanks! Correcting ASAP.
All credit should go to @Trott though ;)

Tiriel · 2017-12-24T13:00:57Z

Done! Thanks!

Trott · 2017-12-24T16:58:17Z

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

Two small nits:
/node/deps looks like a path, so maybe just a build-in module?
Can you add Ref: https://github.com/nodejs/node/issues/17148 to this comment or something like that? Give people a link so they can see the bug that this approach avoids that the simpler approach being replaced doesn't.

True! It's done

Trott · 2017-12-24T17:03:13Z

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

Nit: Maybe restore the error message checking in the catch block?
assert.ok(err.toString().startsWith('Error: Cannot find module ')`

Trott

Thanks for doing this!

Trott · 2017-12-24T17:03:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12287/

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. Fixes: nodejs#17148 Refs: nodejs#17148 (comment)

Tiriel · 2017-12-25T08:13:04Z

Done! Thanks!

TimothyGu · 2017-12-25T10:14:49Z

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

Missing period

TimothyGu · 2017-12-25T10:15:35Z

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

I’d still like to see a continue here to avoid unneeded assertion in the following line.

Fix following review. Reinstated assertion on error message, and corrected comment. Leaving continu to break loop and avoid unneeded assertions. Fixes: nodejs#17148 Refs: nodejs#17148 (comment)

Tiriel · 2017-12-27T08:57:12Z

Sorry for the delay, I was traveling!
I fixed following return, although the diff didn't disappear for some reason... I think I amended my commit instead of simple adding one.

TimothyGu

LGTM. Thanks for staying with this through the back and forth. I think the comment doesn't disappear as it is technically on a line that still exists in its entirety.

jasnell · 2017-12-28T17:12:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12330/

Tiriel · 2017-12-29T08:36:23Z

Are these tests know to be this long/to fail, or is there some thing I can fix on my end?

jasnell · 2017-12-29T15:27:16Z

CI failures are unrelated.

maclover7 · 2018-01-02T23:05:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12380/

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. PR-URL: #17848Fixes: #17148 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

apapirovski · 2018-01-03T19:00:57Z

Landed in 30892c8

MylesBorins · 2018-01-09T23:46:08Z

This does not land cleanly on v9.x, could it be backported?

Tiriel · 2018-01-10T09:12:30Z

If it's something I can do myself, I'll gladly give it a try!

targos · 2018-01-10T09:21:20Z

@Tiriel thanks! The goal would be to checkout a new branch based on upstream/v9.x-staging and one of:

cherry-pick this commit and fix conflicts, then open a new PR
rewrite the change if it has to be very different, then open a new PR
ask us to change the label to "dont-land-on-v9.x" if it is not relevant to this branch

See https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md for the full guide.

Tiriel · 2018-01-10T09:24:31Z

Thanks, I was precisely reading this guide and following it 😄

I'll try these solutions in this order.

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#17848Fixes: nodejs#17148 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

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: #17848Fixes: #17148 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 24, 2017

TimothyGu approved these changes Dec 24, 2017
View reviewed changes

Tiriel force-pushed the test-missing-error branch from 3c3ffe8 to 3976872Compare December 24, 2017 12:56

cjihrig approved these changes Dec 24, 2017
View reviewed changes

Trott reviewed Dec 24, 2017
View reviewed changes

Trott approved these changes Dec 24, 2017
View reviewed changes

Tiriel force-pushed the test-missing-error branch from 3976872 to ea3c32eCompare December 25, 2017 07:55

TimothyGu reviewed Dec 25, 2017
View reviewed changes

tests: fix error message
398246f
Fix following review. Reinstated assertion on error message, and corrected comment. Leaving continu to break loop and avoid unneeded assertions. Fixes: nodejs#17148 Refs: nodejs#17148 (comment)

Tiriel force-pushed the test-missing-error branch from ea3c32e to 398246fCompare December 27, 2017 08:52

TimothyGu approved these changes Dec 27, 2017
View reviewed changes

jasnell approved these changes Dec 27, 2017
View reviewed changes

TimothyGu added dont-land-on-v4.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 3, 2018

apapirovski closed this Jan 3, 2018

MylesBorins added the backport-requested-v9.x label Jan 9, 2018

Tiriel mentioned this pull request Jan 10, 2018
[v9.x backport] test: fix require-deps-deprecation for installed deps #18077
Closed
3 tasks

TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018

MylesBorins added backported-to-v9.x and removed backport-requested-v9.x labels Feb 20, 2018

MylesBorins mentioned this pull request Feb 21, 2018
v9.6.0 proposal #18902
Merged

Uh oh!

tests: fix test-require-deps-deprecation for already installed deps#17848

tests: fix test-require-deps-deprecation for already installed deps #17848

Uh oh!

Conversation

Tiriel commented Dec 24, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tiriel commented Dec 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 24, 2017

Uh oh!

Tiriel commented Dec 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tiriel commented Dec 27, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 28, 2017

Uh oh!

Tiriel commented Dec 29, 2017

Uh oh!

jasnell commented Dec 29, 2017

Uh oh!

maclover7 commented Jan 2, 2018

Uh oh!

apapirovski commented Jan 3, 2018

Uh oh!

MylesBorins commented Jan 9, 2018

Uh oh!

Tiriel commented Jan 10, 2018

Uh oh!

targos commented Jan 10, 2018

Uh oh!

Tiriel commented Jan 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants