test: fix common.PIPE path bug

[Trott]

common.PIPE is returning a path name in test rather than in the
tmp directory for each test. This is causing multiple test failures in
CI. Make the path name inside the temporary directories again. This way
the pipe is removed by common.refreshTmpDir() on POSIX.

The bug in common.PIPE was introduced in
c34ae48.

Fixes: #16290
Fixes: #16323

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

test

[Trott]

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

[Trott]

CI is green (well, yellow), this change is trivial, seems like a very obvious bug fix, and is super-easy to revert in the very unlikely event we'll want to do that. I think this qualifies for expedited landing.

[Trott]

Landed in 8e268c7

[gireeshpunathil]

Can you please elaborate on the change? It does not occur obvious to me.

this line makes exports.tmpDir to be a fully formed temp folder path, and that makes both:

exports.PIPE = pipePrefix + pipeName;
and
exports.PIPE = path.join(pipePrefix, pipeName);

the same.

I tested with a random test file, and the result is same with and without the change:

with exports.PIPE = pipePrefix + pipeName;
$ ./node test/parallel/test-pipe-stream.js
/home/gireesh/cl/node/test/tmp/node-test.9765110.sock

with exports.PIPE = path.join(pipePrefix, pipeName);
$ ./node test/parallel/test-pipe-stream.js
/home/gireesh/cl/node/test/tmp/node-test.9765114.sock

[gireeshpunathil]

Also, even when the PATH is correctly setup, there can be collisions within the temp folder when parallel tests run, and that is what #16324 addresses and is still required I guess.

[Trott]

Also, even when the PATH is correctly setup, there can be collisions within the temp folder when parallel tests run

No, this is incorrect. When tests are run in parallel, they each get their own separate temp directories assigned by test.py. They cannot collide.

[Trott]

this line makes exports.tmpDir to be a fully formed temp folder path, and that makes both:

exports.PIPE = pipePrefix + pipeName;
and
exports.PIPE = path.join(pipePrefix, pipeName);

There is no trailing slash on the temp directory/pipe prefix, so they are not the same.

Before this PR:

> require('./test/common').PIPE
'test/tmpnode-test.57450.sock'
>

After this PR:

> require('./test/common').PIPE
'test/tmp/node-test.57468.sock'
> 

[refack]

with exports.PIPE = pipePrefix + pipeName;
$ ./node test/parallel/test-pipe-stream.js
/home/gireesh/cl/node/test/tmp/node-test.9765110.sock

with exports.PIPE = path.join(pipePrefix, pipeName);
$ ./node test/parallel/test-pipe-stream.js
/home/gireesh/cl/node/test/tmp/node-test.9765114.sock

The bug is in the creating of localRelative

node/test/common/index.js

Line 274 in 83b40de

const localRelative = path.relative(process.cwd(), `${exports.tmpDir}/`);

path.relative strips the last /

[Trott]

@refack Oh shoot, I got the commit wrong. Something was telling me I shouldn't have even included that in the text. I should listen to my gut next time.

[Trott]

Looks like the actual commit where the bug was introduced is 971aad1.

[gireeshpunathil]

@Trott - sorry for the noise. I ran the test with a binary that did not have 971aad1 in it. Once it is added, I am able to follow your route. Thanks!

However, I am not able to follow the uniqueness in terms of common.PIPE under parallel execution. I did the following:

diff --git a/test/parallel/test-pipe-stream.js b/test/parallel/test-pipe-stream.js
index 8fd9d31..3f8241b 100644
--- a/test/parallel/test-pipe-stream.js
+++ b/test/parallel/test-pipe-stream.js
@@ -2,9 +2,12 @@
 common.refreshTmpDir();

+
+fs.appendFile('foo.txt', `\nfilename: ${common.PIPE}`, (e, d) => {});

and

diff --git a/test/parallel/test-pipe-unref.js b/test/parallel/test-pipe-unref.js
index 17caa01..1097b44 100644
--- a/test/parallel/test-pipe-unref.js
+++ b/test/parallel/test-pipe-unref.js
@@ -1,9 +1,12 @@
+const fs = require('fs');

 common.refreshTmpDir();

+fs.appendFile('foo.txt', `\nfilename: ${common.PIPE}`, (e, d) => {});
+
 const s = net.Server();
 s.listen(common.PIPE);
 s.unref();

$ tools/test.py parallel/test-pipe-unref parallel/test-pipe-stream
[00:00|% 100|+   2|-   0]: Done                     
$  cat foo.txt

filename: test/tmp.0/node-test.10485768.sock
filename: test/tmp.0/node-test.10485770.sock
$

As can be seen, the pipes are made unique only by PIDs, not by paths. Am doing anything incorrect?

[Trott]

@gireeshpunathil If tests are invoked as test.py parallel, then -j defaults to 1 and it does not in fact running tests in parallel but one at a time. To run in parallel, pass either -j <INT> or -J:

$ tools/test.py -j 4 parallel
... runs tests in four spearate processes ...
$ 

-j takes an integer that specifies the number of processes. In contrast, the number of processes that -J uses depends on how many processors the machine has.

[Trott]

@gireeshpunathil I wouldn't be surprised iff this isn't particularly well-documented or is documented but easy to miss. Thanks for asking the questions. There are no doubt other folks who don't know this stuff who now maybe do because you asked. :-D

[refack]

@refack Oh shoot, I got the commit wrong. Something was telling me I shouldn't have even included that in the text. I should listen to my gut next time.

I also missed that 🤣 I was just answering @gireeshpunathil. It's good to know that the subtle bug only existed for 16 days (I wanted to query Jenkins trying to find all failures)

[gireeshpunathil]

thanks @Trott for the explanation - I understood it now, and verified it to my satisfaction.

[MylesBorins]

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

[Trott]

@MylesBorins This should be backported if #15988 is backported because it fixes a bug in that PR. Otherwise, this should not be backported.