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

@@ -23,9 +23,12 @@ const deprecatedModules = [

 // Newly added deps that do not have a deprecation wrapper around it would
 // throw an error, but no warning would be emitted.
+// Added false dep to check test throws even if acorn is in the user's
+// resolve tree.


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

+  try {
+    path = require.resolve(m);
+  } catch ($e) {
+    assert.ok($e);


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

+  } catch ($e) {
+    assert.ok($e);
+  }
+  assert.notStrictEqual(path, m);


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

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

Done! Thanks!

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

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

@@ -39,6 +39,15 @@ for (const m of deprecatedModules) {
  } catch (err) {}
 }

+// Instead of checking require, check that resolve isn't pointing toward
+// /node/deps, as user might already have node installed with acorn in
+// require.resolve range


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

+  let path;
+  try {
+    path = require.resolve(m);
+  } catch (err) {


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

@@ -39,6 +39,16 @@ for (const m of deprecatedModules) {
  } catch (err) {}
 }

+// Instead of checking require, check that resolve isn't pointing toward
+// built-in module, as user might already have node installed with acorn in
+// require.resolve range


Missing period

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

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

+  try {
+    path = require.resolve(m);
+  } catch (err) {
+    assert.ok(err.toString().startsWith('Error: Cannot find module '));


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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Tiriel commented Dec 24, 2017

TimothyGu Dec 24, 2017

Tiriel Dec 24, 2017

TimothyGu Dec 24, 2017

TimothyGu Dec 24, 2017

Tiriel Dec 24, 2017

TimothyGu Dec 24, 2017

Tiriel Dec 24, 2017

Tiriel commented Dec 24, 2017

Trott Dec 24, 2017

Tiriel Dec 25, 2017

Trott Dec 24, 2017

Trott left a comment

Trott commented Dec 24, 2017

Tiriel commented Dec 25, 2017

TimothyGu Dec 25, 2017

TimothyGu Dec 25, 2017

Tiriel commented Dec 27, 2017

TimothyGu left a comment

jasnell commented Dec 28, 2017

Tiriel commented Dec 29, 2017

jasnell commented Dec 29, 2017

maclover7 commented Jan 2, 2018

apapirovski commented Jan 3, 2018

MylesBorins commented Jan 9, 2018

Tiriel commented Jan 10, 2018

targos commented Jan 10, 2018

Tiriel commented Jan 10, 2018

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

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

Conversation

Tiriel commented Dec 24, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tiriel commented Dec 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

Trott commented Dec 24, 2017

Tiriel commented Dec 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tiriel commented Dec 27, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

jasnell commented Dec 28, 2017

Tiriel commented Dec 29, 2017

jasnell commented Dec 29, 2017

maclover7 commented Jan 2, 2018

apapirovski commented Jan 3, 2018

MylesBorins commented Jan 9, 2018

Tiriel commented Jan 10, 2018

targos commented Jan 10, 2018

Tiriel commented Jan 10, 2018