parallel/test-worker-process-cwd.js fails when node in root directory

[ssample812]

• Version: Node 12.4.0
• Platform: Ubuntu 18.04.2 LTS

After building Node.js from the source, I ran the test suite and an error for parallel/test-worker-process-cwd.js. The test attempts to move up a directory and throws an error if the cwd is unchanged. However, I had node in my root directory, so the lack of change was not indicative of a problem, but rather the correct result. When I moved node to a lower directory, the test passed. It seems to me that the test should either be changed to work when node is in the root, or skipped with a message stating this stipulation. I'm not very familiar with Node.js or its test suite, so I'm not sure the best implementation of this change.

[addaleax]

Thanks for opening this issue!

As for this particular problem, I think the easiest solution would be to run process.chdir(__dirname); as part of the main thread first, in order to normalize where the test runs and avoid the / problem.

There’s also more issues with the test; at the very least, these lines form a race condition:

node/test/parallel/test-worker-process-cwd.js

Lines 13 to 14 in d9add75

	const w = new Worker(__filename);
	process.chdir('..');

And as a wider problem, I’m having a hard time following the data flow between the individual workers, and the order in which the events happen. It’s also not obvious why there are two workers to begin with just from looking at the test – the reason is that it checks that an optimization in process.cwd() works, but that should be documented as part of what the test does.

If you feel like taking any of this up yourself, feel free to open a pull request, and let us know if you need any help with that!

[addaleax]

@ssample812 Any further thoughts? If you prefer, I’ll try to open a PR myself.

[ssample812]

Thank you for your response! I'm glad this has brought to light other issues with the test. Though I would love to take up these fixes myself, I know very little about Node.js and its test suite and would be entirely ill equipped to do so. Any others ready to tackle the issue I commend you!

[BridgeAR]

@addaleax

I think the easiest solution would be to run process.chdir(__dirname); as part of the main thread first, in order to normalize where the test runs and avoid the / problem

👍

[...] these lines [13-14] form a race condition

Can you elaborate on why it's a race condition? I can't see the issue looking at the code.

And as a wider problem, I’m having a hard time following the data flow between the individual workers, and the order in which the events happen.

It does seem a bit confusing looking at it again. It seems the test could also be simplified in some cases.

It’s also not obvious why there are two workers to begin with just from looking at the test

Documenting the behavior would indeed be good 👍. It's required to verify that worker instances properly hand over the shared counter. One worker alone would not suffice for that.

@ssample812 please feel free to open a PR where you add process.chdir(__dirname); this right above L13 1d022e8#diff-93f829d9b43d32dcdf8e3dc18b075b3aR13. That should at least solve your problem with the test :)

[ssample812]

I have opened a PR: #28271

[addaleax]

[...] these lines [13-14] form a race condition

Can you elaborate on why it's a race condition? I can't see the issue looking at the code.

@BridgeAR Spawning a thread and then changing state that influences the thread’s behaviour is always a race condition. You can e.g. add const start = Date.now(); while (Date.now() - start < 1000); between the two lines to make the timing bad enough to make the test fail reliably.