test: make temp path customizable by jbergstroem · Pull Request #3325 · nodejs/node · GitHub

In CI we previously passed NODE_COMMON_PIPE to the test runner to avoid long filenames on Windows. Avoid long paths by changing the temporary directory instead. This also allows us to run test suites in parallel since NODE_COMMON_PIPE otherwise would have been used from multiple tests.

/R=@nodejs/build, @Trott ?

CI (without passing NODE_TMP_DIR): https://ci.nodejs.org/job/node-test-commit/804/

__dirname perhaps?

I'm still catching up but did NODE_COMMON_PIPE get removed from all the tests already?

AFAIK it was never in tests, just passed to exports.PIPE [edit: if passed to common.js].

I'll leave it to someone more in tune with the entirety of the build process to LGTM or not, but I'm +1. The common.PIPE improvements that build on this to enable parallel testing will be in a subsequent PR?

Prior to removal, if NODE_COMMON_PIPE was unset exports.PIPE would be stored in the temporary directory which works fine in parallel runs since the thread id is appended to common.tmpDir. The change I'm introducing is the ability to change where tmpDir lives.

@jbergstroem ... what's the status on this? Looks like it needs to be rebased at a minimum

@jasnell waiting for feedback/review. I'll rebase.

@Trott to answer your question regarding "enabling" parallel builds -- that would be something we'd most likely be changing in the jenkins setup.

LGTM

Can you ensure that testRoot exists? Since this is now a variable, it can be useful to set it to a directory that does not exist yet.

To run parallel tests on Windows, the pipe name should also depend on the thread number. What about adding something like lines 64 and 66 after 139?

@joaocgreis does the pipe really have to depend on a thread number since it already lives in its own thread folder (tmp.$thread)?

Regarding creating the directory, that's already part of refreshTmpDir.

@jbergstroem the pipe is defined differently in windows, it is a global name, not a file in the test folder (as created in line 139).

The tmp.$thread directories are created by refreshTmpDir, but testRoot is not. Currently this is not a problem since the test directory always exists, but if you set NODE_TEST_DIR to a directory that does not exit, tests fail. There could be a mkdir(testRoot) or just a descriptive exception if it does not exist, but on the other hand the error is pretty clear so feel free to ignore this suggestion.

Rebased and fixed an issue with two tests that assumed a specific folder structure. CI (which again shouldn't branch into this): https://ci.nodejs.org/job/node-test-commit/1324/

Oh, and @joaocgreis -- I now create the dir through refreshTmp.

existsSync() is deprecated so maybe use one of the suggested alternatives?

ping!

needs space between + and '.'

should be +=

@jbergstroem wouldn't checking the NODE_TEST_DIR directly in the test runner be better?

@joaocgreis checking in the runner now. Also, created an option called --test-dir supplementing NODE_TEST_DIR.

CI (should be unaffected): https://ci.nodejs.org/job/node-test-commit/1560/

In CI we previously passed `NODE_COMMON_PIPE` to the test runner to avoid long filenames. Add an option to the test runner that allows the user to change the temporary directory instead. This also allows us to run test suites in parallel since `NODE_COMMON_PIPE` otherwise would have been used from multiple tests at the same time. PR-URL: #3325 Reviewed-By: Joao Reis <reis@janeasystems.com>

A few tests assumed that temp dirs always lived in the same parent folder as fixtures. Make these use `common.tmpDir` instead. PR-URL: #3325 Reviewed-By: Joao Reis <reis@janeasystems.com>

Landed in v4.x-staging in d33279d and b3079ef

In CI we previously passed `NODE_COMMON_PIPE` to the test runner to avoid long filenames. Add an option to the test runner that allows the user to change the temporary directory instead. This also allows us to run test suites in parallel since `NODE_COMMON_PIPE` otherwise would have been used from multiple tests at the same time. PR-URL: #3325 Reviewed-By: Joao Reis <reis@janeasystems.com>

A few tests assumed that temp dirs always lived in the same parent folder as fixtures. Make these use `common.tmpDir` instead. PR-URL: #3325 Reviewed-By: Joao Reis <reis@janeasystems.com>

In CI we previously passed `NODE_COMMON_PIPE` to the test runner to avoid long filenames. Add an option to the test runner that allows the user to change the temporary directory instead. This also allows us to run test suites in parallel since `NODE_COMMON_PIPE` otherwise would have been used from multiple tests at the same time. PR-URL: nodejs#3325 Reviewed-By: Joao Reis <reis@janeasystems.com>

A few tests assumed that temp dirs always lived in the same parent folder as fixtures. Make these use `common.tmpDir` instead. PR-URL: nodejs#3325 Reviewed-By: Joao Reis <reis@janeasystems.com>

jbergstroem added the build Issues and PRs related to build files or the CI. label Oct 12, 2015

rvagg reviewed Oct 12, 2015
View reviewed changes

mscdex added test Issues and PRs related to the tests. and removed build Issues and PRs related to build files or the CI. labels Oct 12, 2015

jbergstroem force-pushed the feature/rework_common_pipe branch from cc1749a to 3f1add0 Compare October 22, 2015 23:28

jbergstroem mentioned this pull request Nov 20, 2015

Retry long path tests on Linux under real tmp dir #3929

Closed

jbergstroem force-pushed the feature/rework_common_pipe branch from 3f1add0 to 0740b42 Compare December 3, 2015 03:30

Trott reviewed Dec 3, 2015
View reviewed changes

rvagg reviewed Dec 8, 2015
View reviewed changes

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

jbergstroem force-pushed the feature/rework_common_pipe branch from 00d9d98 to f96e027 Compare December 29, 2015 01:55

Trott mentioned this pull request Jan 15, 2016

test: write to tmp dir rather than fixture dir #4489

Closed

jasnell added land-on-v4.x and removed lts-watch-v4.x labels Jan 15, 2016

MylesBorins mentioned this pull request Jan 15, 2016

Audit commits not found on v4.x-staging #4698

Closed

richardlau mentioned this pull request Jan 15, 2016

test: don't use cwd for relative path #4477

Merged

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

MylesBorins added lts-landed-on-v4.x and removed land-on-v4.x labels Jan 28, 2016

MylesBorins removed the lts-agenda label Feb 19, 2016

joaocgreis mentioned this pull request Apr 7, 2016

tools: remove unneeded temp directory handling #6094

Closed

2 tasks

This was referenced May 13, 2024

[Snyk] Security upgrade karma from 3.1.4 to 4.2.0 leonardoadame/node#821

Open

[Snyk] Security upgrade karma from 3.1.4 to 4.2.0 leonardoadame/node#851

Open

This was referenced May 14, 2024

[Snyk] Security upgrade karma from 3.1.4 to 4.2.0 aliscco/alisco-node#1462

Open

[Snyk] Security upgrade karma from 3.1.4 to 4.2.0 aliscco/alisco-node#1472

Open

[Snyk] Security upgrade karma from 3.1.4 to 4.2.0 aliscco/alisco-node#1482

Open

leonardoadame mentioned this pull request May 14, 2024

[Snyk] Security upgrade karma from 3.1.4 to 4.2.0 leonardoadame/node#892

Open

This was referenced May 14, 2024

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#1491

Open

[Snyk] Security upgrade karma from 3.1.4 to 4.2.0 aliscco/alisco-node#1548

Open

[Snyk] Security upgrade karma from 3.1.4 to 4.2.0 aliscco/alisco-node#1549

Open

This was referenced May 22, 2024

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#1632

Open

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#1639

Open

This was referenced May 23, 2024

[Snyk] Security upgrade karma from 3.1.4 to 4.2.0 leonardoadame/node#1033

Open

[Snyk] Security upgrade karma from 3.1.4 to 4.2.0 leonardoadame/node#1039

Open

test: make temp path customizable by jbergstroem · Pull Request #3325 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: make temp path customizable - #3325

test: make temp path customizable#3325jbergstroem merged 2 commits intonodejs:masternodejs/node:masterfrom jbergstroem:feature/rework_common_pipejbergstroem/node:feature/rework_common_pipeCopy head branch name to clipboard

Conversation

jbergstroem commented Oct 12, 2015

Uh oh!

jbergstroem commented Oct 12, 2015

Uh oh!

rvagg Oct 12, 2015

Choose a reason for hiding this comment

Uh oh!

rvagg commented Oct 12, 2015

Uh oh!

jbergstroem commented Oct 12, 2015

Uh oh!

Trott commented Oct 12, 2015

Uh oh!

jbergstroem commented Oct 12, 2015

Uh oh!

jasnell commented Oct 22, 2015

Uh oh!

jbergstroem commented Oct 22, 2015

Uh oh!

jbergstroem commented Oct 26, 2015

Uh oh!

joaocgreis commented Oct 28, 2015

Uh oh!

jbergstroem commented Nov 2, 2015

Uh oh!

joaocgreis commented Nov 3, 2015

Uh oh!

jbergstroem commented Dec 3, 2015

Uh oh!

jbergstroem commented Dec 3, 2015

Uh oh!

Trott Dec 3, 2015

Choose a reason for hiding this comment

Uh oh!

jbergstroem commented Dec 8, 2015

Uh oh!

rvagg Dec 8, 2015

Choose a reason for hiding this comment

Uh oh!

joaocgreis Dec 8, 2015

Choose a reason for hiding this comment

Uh oh!

joaocgreis commented Dec 9, 2015

Uh oh!

jbergstroem commented Dec 29, 2015

Uh oh!

jbergstroem commented Dec 29, 2015

Uh oh!

jasnell commented Jan 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

test: make temp path customizable#3325
jbergstroem merged 2 commits into
nodejs:masterfrom
jbergstroem:feature/rework_common_pipe