Support NODE_PATH in forked process #531

ingro · 2016-02-09T09:14:23Z

This PR tries to fix #515 in a non-destructive way.

Basically it checks for the presence of NODE_PATH env variable before forking the process and creates an absolute path from it, so it could be used in test-worker.js as additional node module path.

That won't work there because process.cwd() points to the forked process test file.

mention-bot · 2016-02-09T09:14:25Z

By analyzing the blame information on this pull request, we identified @novemberborn, @ariporad and @sotojuan to be potential reviewers

novemberborn · 2016-02-09T12:23:30Z

Rather than modifying process.env.NODE_PATH in api.js you should probably pass a custom value in fork.js: https://github.com/sindresorhus/ava/blob/bdb8b6ebaa05f1dc31cdab7c84aed7d92317b8ed/lib/fork.js#L19:L22

novemberborn · 2016-02-09T12:24:47Z

lib/test-worker.js

@@ -86,6 +86,9 @@ var oldNodeModulesPaths = module.constructor._nodeModulePaths;
 module.constructor._nodeModulePaths = function () {
 	var ret = oldNodeModulesPaths.apply(this, arguments);
 	ret.push(nodeModulesDir);
+	if (process.env.NODE_PATH) {
+		ret.push(process.env.NODE_PATH);


NODE_PATH can contain multiple values. Not sure if the env var needs to be split and concatenated with ret here. See https://nodejs.org/api/modules.html#modules_loading_from_the_global_folders for more.

ingro · 2016-02-09T13:55:41Z

Thanks for your inputs @novemberborn , I made the following changes:

removed the ugly process.env.NODE_PATH override in api.js
added an option for the forked process (projectRoot) which contains the original path from where ava was called;
cycled through possible multiple values of process.env.NODE_PATH (with a simple OS check) and join their path with the one supplied by projectRoot.

novemberborn · 2016-02-09T14:04:26Z

lib/fork.js

@@ -13,7 +13,8 @@ module.exports = function (file, opts) {
 		tty: process.stdout.isTTY ? {
 			columns: process.stdout.columns,
 			rows: process.stdout.rows
-		} : false
+		} : false,
+		projectRoot: process.cwd()


This isn't necessarily the project root. Perhaps parentWorkingDirectory? Bit verbose though.

What about parentCwd?

novemberborn · 2016-02-09T14:18:53Z

Perhaps resolve the additional, absolute NODE_PATHs in the main process, once, and then pass the array through the opts to the test-worker.

You should add a test, probably in test/fork.js. Note that tests are written using node-tap. Maybe use t.spawn to spawn another test file (from the fixtures directory) with a NODE_PATH environment variable, then in that file use the lib/fork module to run an AVA test. You could set up NODE_PATH so the final test can do an import and have it be resolved correctly. I'm in Gitter if that wasn't clear https://gitter.im/sindresorhus/ava

…ATH env

novemberborn · 2016-02-09T15:23:21Z

lib/fork.js

@@ -13,7 +13,8 @@ module.exports = function (file, opts) {
 		tty: process.stdout.isTTY ? {
 			columns: process.stdout.columns,
 			rows: process.stdout.rows
-		} : false
+		} : false,
+		additionalPaths: opts.additionalPaths


Object.assign() already takes care of mixing in the opts.

ingro · 2016-02-10T09:27:31Z

I added the test and it seems like it's green now, but I surely forgot something else, let me know if something is missing!

shinnn · 2016-02-10T09:47:34Z

cli.js

@@ -93,11 +94,22 @@ if (cli.flags.init) {
 	return;
 }

+var nodePaths;
+if (process.env.NODE_PATH) {
+    var osSplitChar = process.platform === 'win32' ? ';' : ':';


Instead you can use path.delimiter. https://nodejs.org/api/path.html#path_path_delimiter

Cool! So I can get rid of all the env check stuff!

I should have commented it earlier :)

novemberborn · 2016-02-11T15:09:05Z

@ingro I think what @sindresorhus is saying is that you can add an env object in fork.js which sets NODE_PATH to the corrected value. Then you don't need the changes in test-worker.js.

sindresorhus · 2016-02-11T15:53:12Z

Yup, and keep in mind that the whole existing process.env needs to be passed to the fork thing to preserve existing behavior, so use object-assign to copy the object and then modify the NODE_PATH key.

ingro · 2016-02-11T16:56:48Z

Well I think that's cleaner, I will look at it as soon as I have time!

ingro · 2016-02-12T16:31:00Z

The commit has some conflicts, what's the procedure in situation like this?

sindresorhus · 2016-02-13T15:33:00Z

@ingro You resolve the merge conflict. (No need to open a new PR)

ingro · 2016-02-15T11:40:17Z

It seems we are back to green!

novemberborn · 2016-02-15T11:42:19Z

lib/fork.js

+			.map(function (p) {
+				return path.resolve(p);
+			})
+			.join(path.delimiter);


Could compute this outside of the function so it can be reused for subsequent forks.

sindresorhus · 2016-02-16T11:09:22Z

lib/fork.js

+
+	env = objectAssign({
+		NODE_PATH: nodePath
+	}, env);


Ugh, discovered this too late. This makes no sense. Here you overwrite the modified NODE_PATH with the original one. This means the tests are also invalid. Tests still pass when the .map is commented out.

// @ingro @novemberborn

ingro · 2016-02-16T11:40:10Z

Welp sorry, I blindly copied from one of your comment above.
That should fix the wrong assignment:
env = objectAssign(env, { NODE_PATH: nodePath });

Anyway tests still fails, will look into that!

ingro · 2016-02-16T14:24:34Z

I think I found the problem with the test, it was:

var nodePaths = 'node-paths/modules' + path.delimiter + 'node-paths/deep/nested';

while it should have been:

var nodePaths = 'fixture/node-paths/modules' + path.delimiter + 'fixture/node-paths/deep/nested';

That's because NODE_PATH should be relative to the directory from which ava is running, not to the test itself.

I've fixed that in my fork, I should create another pull request?

novemberborn · 2016-02-16T14:25:07Z

I should create another pull request?

Yep! 👯

ingro · 2016-02-16T14:36:55Z

Created here #559

hildjj · 2021-02-11T21:56:14Z

@sindresorhus I think this regressed with commit 1f10a7c.

novemberborn · 2021-02-12T08:41:35Z

@hildjj that was intentional.

hildjj · 2021-02-12T15:11:40Z

Is this the correct place to discuss whether that was the correct decision? I don't see an issue or pull request associated with that commit, so I'll assume yes, unless you'd like me to open a new issue.

I have an (admittedly crappy) use case where being able to manipulate NODE_PATH is very handy. It involves using the same test suite to test two different builds of the same software that have subtle differences in their dependencies. I can figure out another way around this if I have to, but it was convenient.

novemberborn · 2021-02-12T15:28:51Z

You could use an ava.config.cjs file to rewrite the NODE_PATH and configure it as an environment variable for AVA to use for the worker processes. We're not going to bring back specialized handling in AVA itself.

hildjj · 2021-02-12T15:44:49Z

I'm arguing that it's not specialized handling. ava is now modifying the environment being passed to my tests in a surprising way. At the very least I would have expected something in the release notes and something in the docs for a change that breaks existing functionality built in to node. The docs currently say:

"environmentVariables: specifies environment variables to be made available to the tests. The environment variables defined here override the ones from process.env"

which is at least confusing at the moment, since it's not clear that the process.env isn't the same as the environment passed in to ava.

ava.config.cjs might work for me, with some logic to detect which of the cases I'm in, I suppose.

novemberborn · 2021-02-12T15:47:03Z

No, AVA 3 no longer modifies anything. But let's continue this in a Discussion.

hildjj · 2021-02-12T16:10:17Z

#2682 for linkage

Support NODE_PATH in forked process

c2aab4f

novemberborn reviewed Feb 9, 2016
View reviewed changes

inyono mentioned this pull request Feb 9, 2016

[WIP] Set cwd of forks to parent process.cwd() #524

Closed

Reworked how to pass the project path to the forked process

3704a07

ingro added 2 commits February 9, 2016 15:00

Reworked how to pass the project path to the forked process

33dd8f6

Fix linting

669be7c

novemberborn reviewed Feb 9, 2016
View reviewed changes

Update logic to generate an array of additional paths from the NODE_P…

d37a356

…ATH env

novemberborn reviewed Feb 9, 2016
View reviewed changes

Changed options name and added test to support NODE_PATH

082708b

ingro force-pushed the master branch from bc3feba to 082708b Compare February 9, 2016 16:54

ingro added 3 commits February 9, 2016 18:03

Partially fixed tests

d4bf3ee

Fix cli test for NODE_PATH support

1e2d8ad

Fix test for NODE_PATH on all os

eecfebb

shinnn reviewed Feb 10, 2016
View reviewed changes

Use path.delimiter instead of check os

c42afed

ingro added 2 commits February 12, 2016 11:46

New approach to retrieve NODE_PATH from forked process

5dc109e

Linting

f947eb0

ingro added 3 commits February 15, 2016 11:57

Merge to resolve conflicts

f932f07

Linting

d2fa2a2

Fix build APPVEYOR

efdc4c2

novemberborn reviewed Feb 15, 2016
View reviewed changes

Moved env generation for fork process outside

75b0de0

sindresorhus closed this in 0675d34 Feb 16, 2016

sindresorhus reviewed Feb 16, 2016
View reviewed changes

sindresorhus added a commit that referenced this pull request Feb 16, 2016

cleanup #531

69dbcf2

ingro mentioned this pull request Feb 16, 2016

Fix wrong handling of NODE_PATH and relative test #559

Closed

sindresorhus mentioned this pull request Apr 26, 2016

Should process.cwd() be changed to the directory of the currently running test? #32

Closed

jgkim mentioned this pull request Jul 13, 2016

Support NODE_ICU_DATA in forked processes #962

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support NODE_PATH in forked process #531

Support NODE_PATH in forked process #531

ingro commented Feb 9, 2016

mention-bot commented Feb 9, 2016

novemberborn commented Feb 9, 2016

novemberborn Feb 9, 2016

ingro commented Feb 9, 2016

novemberborn Feb 9, 2016

ingro Feb 9, 2016

novemberborn Feb 9, 2016

novemberborn commented Feb 9, 2016

novemberborn Feb 9, 2016

ingro commented Feb 10, 2016

shinnn Feb 10, 2016

ingro Feb 10, 2016

shinnn Feb 10, 2016

novemberborn commented Feb 11, 2016

sindresorhus commented Feb 11, 2016

ingro commented Feb 11, 2016

ingro commented Feb 12, 2016

sindresorhus commented Feb 13, 2016

ingro commented Feb 15, 2016

novemberborn Feb 15, 2016

sindresorhus Feb 16, 2016

ingro commented Feb 16, 2016

ingro commented Feb 16, 2016

novemberborn commented Feb 16, 2016

ingro commented Feb 16, 2016

hildjj commented Feb 11, 2021

novemberborn commented Feb 12, 2021

hildjj commented Feb 12, 2021

novemberborn commented Feb 12, 2021

hildjj commented Feb 12, 2021

novemberborn commented Feb 12, 2021

hildjj commented Feb 12, 2021

Support NODE_PATH in forked process #531

Support NODE_PATH in forked process #531

Conversation

ingro commented Feb 9, 2016

mention-bot commented Feb 9, 2016

novemberborn commented Feb 9, 2016

Choose a reason for hiding this comment

ingro commented Feb 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn commented Feb 9, 2016

Choose a reason for hiding this comment

ingro commented Feb 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn commented Feb 11, 2016

sindresorhus commented Feb 11, 2016

ingro commented Feb 11, 2016

ingro commented Feb 12, 2016

sindresorhus commented Feb 13, 2016

ingro commented Feb 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingro commented Feb 16, 2016

ingro commented Feb 16, 2016

novemberborn commented Feb 16, 2016

ingro commented Feb 16, 2016

hildjj commented Feb 11, 2021

novemberborn commented Feb 12, 2021

hildjj commented Feb 12, 2021

novemberborn commented Feb 12, 2021

hildjj commented Feb 12, 2021

novemberborn commented Feb 12, 2021

hildjj commented Feb 12, 2021