test: make temp path customizable #3325

jbergstroem · 2015-10-12T06:07:56Z

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 ?

jbergstroem · 2015-10-12T06:10:55Z

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

rvagg · 2015-10-12T10:54:17Z

test/common.js

@@ -6,6 +6,8 @@ var assert = require('assert');
 var os = require('os');
 var child_process = require('child_process');

+const testRoot = process.env.NODE_TEST_DIR || path.dirname(__filename);


__dirname perhaps?

rvagg · 2015-10-12T10:55:44Z

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

jbergstroem · 2015-10-12T13:14:51Z

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

Trott · 2015-10-12T15:31:09Z

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?

jbergstroem · 2015-10-12T19:47:11Z

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.

jasnell · 2015-10-22T23:15:49Z

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

jbergstroem · 2015-10-22T23:23:55Z

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

jbergstroem · 2015-10-26T03:48:18Z

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

joaocgreis · 2015-10-28T11:45:04Z

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?

jbergstroem · 2015-11-02T12:20:40Z

@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.

joaocgreis · 2015-11-03T01:53:56Z

@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.

jbergstroem · 2015-12-03T03:56:59Z

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/

jbergstroem · 2015-12-03T03:59:05Z

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

Trott · 2015-12-03T17:35:18Z

test/common.js

@@ -64,18 +66,19 @@ function rmdirSync(p, originalEr) {
 }

 exports.refreshTmpDir = function() {
+  if (!fs.existsSync(testRoot)) {


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

jbergstroem · 2015-12-08T10:27:26Z

ping!

rvagg · 2015-12-08T11:36:45Z

test/common.js

@@ -155,21 +162,13 @@ Object.defineProperty(exports, 'hasFipsCrypto', {

 if (exports.isWindows) {
  exports.PIPE = '\\\\.\\pipe\\libuv-test';
+  if (process.env.TEST_THREAD_ID) {
+    exports.PIPE = +'.' + process.env.TEST_THREAD_ID;


needs space between + and '.'

should be +=

joaocgreis · 2015-12-09T16:06:10Z

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

jbergstroem · 2015-12-29T01:56:25Z

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

jbergstroem · 2015-12-29T02:03:22Z

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>

jasnell · 2016-01-15T06:19:01Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: make temp path customizable #3325

test: make temp path customizable #3325

jbergstroem commented Oct 12, 2015

jbergstroem commented Oct 12, 2015

rvagg Oct 12, 2015

rvagg commented Oct 12, 2015

jbergstroem commented Oct 12, 2015

Trott commented Oct 12, 2015

jbergstroem commented Oct 12, 2015

jasnell commented Oct 22, 2015

jbergstroem commented Oct 22, 2015

jbergstroem commented Oct 26, 2015

joaocgreis commented Oct 28, 2015

jbergstroem commented Nov 2, 2015

joaocgreis commented Nov 3, 2015

jbergstroem commented Dec 3, 2015

jbergstroem commented Dec 3, 2015

Trott Dec 3, 2015

jbergstroem commented Dec 8, 2015

rvagg Dec 8, 2015

joaocgreis Dec 8, 2015

joaocgreis commented Dec 9, 2015

jbergstroem commented Dec 29, 2015

jbergstroem commented Dec 29, 2015

jasnell commented Jan 15, 2016

test: make temp path customizable #3325

test: make temp path customizable #3325

Conversation

jbergstroem commented Oct 12, 2015

jbergstroem commented Oct 12, 2015

rvagg Oct 12, 2015

Choose a reason for hiding this comment

rvagg commented Oct 12, 2015

jbergstroem commented Oct 12, 2015

Trott commented Oct 12, 2015

jbergstroem commented Oct 12, 2015

jasnell commented Oct 22, 2015

jbergstroem commented Oct 22, 2015

jbergstroem commented Oct 26, 2015

joaocgreis commented Oct 28, 2015

jbergstroem commented Nov 2, 2015

joaocgreis commented Nov 3, 2015

jbergstroem commented Dec 3, 2015

jbergstroem commented Dec 3, 2015

Trott Dec 3, 2015

Choose a reason for hiding this comment

jbergstroem commented Dec 8, 2015

rvagg Dec 8, 2015

Choose a reason for hiding this comment

joaocgreis Dec 8, 2015

Choose a reason for hiding this comment

joaocgreis commented Dec 9, 2015

jbergstroem commented Dec 29, 2015

jbergstroem commented Dec 29, 2015

jasnell commented Jan 15, 2016