test: pipe requires tmpdir

[Trott]

Fixes #3227

Except on Windows, common.PIPE is in the temp directory. So tests that use common.PIPE need to call common.refreshTmpDir(). I dislike the non-obvious coupling of those two things, and maybe there's a better solution, but for the moment at least, this fixes it.

[Trott]

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

[Fishrock123]

I dislike the non-obvious coupling of those two things, and maybe there's a better solution, but for the moment at least, this fixes it.

I think this is the more important point. I think this is a better approach: Fishrock123@34daef2

(Writable should allow things to overwrite it as if it were a regular value, I think?)

[Fishrock123]

Basically, minimizing the institutional knowledge, whether linted or not, is more important in the long run.

[Fishrock123]

(You'd still need these patches though I think.)

Also this still only works if you refresh the tmpdir first, for I think obvious reasons. I'm not sure if that is practical to track.

[Trott]

@Fishrock123 I'm trying to think of ways to decouple the use of common.PIPE from a need to refresh the temp directory. We could:

• Move the temp directory refresh back into the Python wrapper. I don't like that idea because I'd like to see less Python and more JS, but the fact is that it will work. Minor down side there is that it won't work if you are running the test from the command line (rather than via the Makefile) and the temp dir does not exist (and maybe other situations like the pipe already exists and is in use or something).
• Move the pipe out of the test-level temp dir and move it to the system temp dir. We'd need a safe way to generate a different name for it each run, similar to POSIX tmpnam() or else all tests that use common.PIPE need to be moved out of test/parallel.

[rvagg]

@jbergstroem has been talking about turning PIPE into a directory so we can parallelise more tests, there's likely some overlap with that discussion and this PR

[jbergstroem]

Re above: @Trott On top of your stuff, I'd like to expose an option to pass a directory in where the temporary directories/files are stored and create/clean that when starting the test runner. Once this is made we should just remove all logic related to NODE_COMMON_PIPE since that breaks parallel runs. We lean on this option in CI.

[rvagg]

Just for background on NODE_COMMON_PIPE for those editing it—it's there because the full path name can be too long for creating unix pipes which have a character limit, this is acute when run in Jenkins because the directories get deep and it's compounded by verbose naming of jobs and slaves. So on Jenkins we set it to ~/test.pipe (not actually ~) cause we know that's short enough.

Not that I see any of this impacting on these changes, just wanted to make sure y'all have that in mind when messing around there.

[Trott]

All of that sounds great to me.

None of the above alters the need for changes in this PR, does it?

[jbergstroem]

@Trott Shouldn't. If so, I'll just modify post landing this. LGTM btw.

[Trott]

Landed in a1040f2

[jasnell]

@Trott @jbergstroem @rvagg ... should this land in v4.x also?

[Trott]

@jasnell It's a bugfix for tests only so... ¯_(ツ)_/¯ Probably fine either way.

[jasnell]

Landed in v4.x in 7a5ae34