test: missing expected exception in parallel/test-require-deps-deprecation.js

[Tiriel]

• Version: nodejs/node:master
• Platform: Linux 4.10.0-38-generic rename node.js -> io.js #42~16.04.1-Ubuntu SMP Tue Oct 10 16:32:20 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: test

Hey there!

Got this error when running tests on master:

$make test-parallel
[...]
/usr/bin/python2.7 tools/test.py --mode=release parallel -J
=== release test-require-deps-deprecation ===                                  
Path: parallel/test-require-deps-deprecation
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at innerThrows (assert.js:186:7)
    at Function.throws (assert.js:205:3)
    at Object.<anonymous> (/home/benjamin/Dev/javascript/NodeFoundation/node/test/parallel/test-require-deps-deprecation.js:43:10)
    at Module._compile (module.js:644:30)
    at Object.Module._extensions..js (module.js:655:10)
    at Module.load (module.js:563:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Function.Module.runMain (module.js:685:10)
    at startup (bootstrap_node.js:192:16)
Command: out/Release/node /home/benjamin/Dev/javascript/NodeFoundation/node/test/parallel/test-require-deps-deprecation.js
[00:51|% 100|+ 1719|-   1]: Done                                               
Makefile:242 : la recette pour la cible « test-parallel » a échouée
make: *** [test-parallel] Erreur 1

If I'm corect this was added some three days ago in this commit 7ce6d23 but I have no idea why that doesn't throws, when the patch was specifically designed to make this kind of require throw.

Is it only me?

For the record, I thought I had done some breaking changes without wanting it, so I erase my copy and re cloned it, fetched the last updates and then ran ./configure and make. Build is as fresh as it can get.

[Trott]

@Tiriel What's the output if you run this?:

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

[Trott]

@Tiriel Also, what's the output of each of these two commands?:

./node -e "require('acorn/dist/acorn')"

./node -e "require('acorn/dist/walk')"

[Tiriel]

Hey @Trott , Thanks!

• test only :

$ ./node test/parallel/test-require-deps-deprecation.js
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at innerThrows (assert.js:186:7)
    at Function.throws (assert.js:205:3)
    at Object.<anonymous> (/home/benjamin/Dev/javascript/NodeFoundation/node/test/parallel/test-require-deps-deprecation.js:43:10)
    at Module._compile (module.js:644:30)
    at Object.Module._extensions..js (module.js:655:10)
    at Module.load (module.js:563:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Function.Module.runMain (module.js:685:10)
    at startup (bootstrap_node.js:192:16)

• require('acorn/dist/acorn') :
  no output
• require('acorn/dist/walk') :
  no output
• Just to be sure, require('acorn/dist/foo') :

module.js:547
    throw err;
    ^

Error: Cannot find module 'acorn/dist/foo'
    at Function.Module._resolveFilename (module.js:545:15)
    at Function.Module._load (module.js:475:25)
    at Module.require (module.js:588:17)
    at require (internal/module.js:11:18)
    at [eval]:1:1
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at Object.runInThisContext (vm.js:152:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:644:30)
    at evalScript (bootstrap_node.js:471:27)

[Trott]

require('acorn/dist/acorn) and require('acorn/dist/walk') should throw like require('acorn/dist/foo') does. Maybe you have acorn tucked away somewhere that require() is finding it? You don't have NODE_PATH environment variable set by any chance, do you?

[Tiriel]

Damn, you got it. Sorry I missed this.
NODE_PATH is indeed defined, though I don't know how to solve this simply. Anyway, not a global issue then, I'll close this.

Thanks!

[Trott]

@Tiriel It's conceivable that a sensible solution is to PR in a change to Makefile/vcbuild.bat or test.py that unsets NODE_PATH. I'm not 100% sure about that--perhaps there's something in CI that needs NODE_PATH but I don't think so.

[Tiriel]

@Trott But it would then need to be re set afterwards right? I'm not really good at sysadmin, but can it be unset temporarily?

[Trott]

@Trott But it would then need to be re set afterwards right? I'm not really good at sysadmin, but can it be unset temporarily?

You can unset it for the subprocess only. It won't affect the invoking shell.

$ NODE_PATH='foo' ./node -e 'console.log(process.env.NODE_PATH)'
foo
$ echo $NODE_PATH

$

[Tiriel]

@Trott Awesome, thanks! And sorry for being such a weight ^^'

[Tiriel]

I'll just reopen this since it seems there may be a solution and a fix that would solve the problem for people potentially having it like me.

Right now I'm investigatin the problem, but prefixing the commands in the Makefile hasn't improved the situation yet. I think because the Python is launching the tests by executing commands, and so the temporary assignment is lost. 4ill try and dig inside test.py just to be sure. Also tried adding process.env.NODE_PATH = ''; at the top of the js test, but no joy.

If anyone as another idea, they're deeply welcome to share!

[Trott]

@Tiriel There's already code in test.py to remove NODE_PATH from the environment before exectuing tests:

  # Remove NODE_PATH
  if "NODE_PATH" in env_copy:
    del env_copy["NODE_PATH"]

Perhaps the root cause of the problem is something else. Maybe check for the other things listed in https://nodejs.org/api/modules.html#modules_loading_from_the_global_folders that confound module loading:

1: $HOME/.node_modules
2: $HOME/.node_libraries
3: $PREFIX/lib/node

(I think $PREFIX will be the value of prefix_node in config.gypi.)

[richardlau]

You can also set NODE_DEBUG=module environment variable to see where Node.js is looking to load modules from.

[Tiriel]

Awesome thanks both of you! I'll check into it ASAP

[Tiriel]

Okay, this was a long time ago, but I finally had the time to put some more tests to it.

Simply checked require.resolve('acorn/dist/acorn')) in test-require-deps-deprecation.js. It appears it's loaded from $HOME/node_modules (without the dot).
To be a bit more specific, require.resolve.paths('/acorn/dist/acorn') indicates that it finds acorn simply by iterating up through my folder structure, searching for a node_module folder at every depth until it finds one. Which is precisely it's usual workflow if I'm not mistaken.

But I must admit I'm note sure which path to follow from now on. I'm searching for a way to limit the upward iterating to a certain number of levels, at least locally in this test file, but it seems weird.

Any intake, or should I just drop it and ignore this test failing when building Node?

[Trott]

@Tiriel I propose this: Change the require() in the test to be require.resolve(). If it throws, the test passes. If it returns a string other than acorn/dist/acorn, then it passes. If it returns exactly acorn/dist/acorn, then it fails.

I'm happy to put that together as a PR, but would be even happier to see you do it. Let me know. :-D

[Tiriel]

@Trott awesome thanks !

I’ll open a PR later today !