tests: fix test-require-deps-deprecation for already installed deps by Tiriel · Pull Request #17848 · nodejs/node · GitHub

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

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.

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

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

To avoid comparing things to the empty string this could be refactored as:

let path;
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 ;)

Done! Thanks!

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

Nit: Maybe restore the error message checking in the catch block?

  assert.ok(err.toString().startsWith('Error: Cannot find module ')`

Thanks for doing this!

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)

Done! Thanks!

Missing period

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)

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.

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.

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

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

CI failures are unrelated.

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: #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>

Landed in 30892c8

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

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

@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.

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#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>

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>

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 3976872 Compare 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 ea3c32e Compare 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 398246f Compare 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 outstanding review comments, 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 outstanding review comments, 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

tests: fix test-require-deps-deprecation for already installed deps by Tiriel · Pull Request #17848 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

tests: fix test-require-deps-deprecation for already installed deps#17848Tiriel wants to merge 2 commits intonodejs:masternodejs/node:masterfrom Tiriel:test-missing-errorTiriel/node:test-missing-errorCopy head branch name to clipboard

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

tests: fix test-require-deps-deprecation for already installed deps#17848
Tiriel wants to merge 2 commits into
nodejs:masterfrom
Tiriel:test-missing-error