test: create temp dir in common.js #1877

Trott · 2015-06-03T03:48:25Z

For tests run outside the python harness (for example to
see console.error() messages for debugging), common.js
should create the testing temp directory if it does not
exist.

ref: #1876

This change does not seem to result in a significant performance hit as far as I can tell, but I welcome better benchmarks than my "run time make test-parallel" methodology.

Fishrock123 · 2015-06-03T03:54:51Z

test/common.js

+// Create tmpDir if it does not exist
+if (! fs.existsSync(exports.tmpDir)) {
+  fs.mkdirSync(exports.tmpDir);
+}


This will print a deprecation warning. Should probably use fs.acessSync.

Or just try { fs.mkdirSync(exports.tmpDir) } catch (ex) {}?

Hmmm...that will require a try/catch block which might have a more substantial affect on performance. Will try it anyway and see...

Well technically fs.mkdirSync() could still throw too, for example if permissions are wrong somehow. I doubt it there would be a measurable impact for tests, but even if there was, there's always moving it to a separate function I suppose to limit the scope of the deopt...

Trott · 2015-06-03T04:51:58Z

New commit pushed to use fs.accessSync() instead of deprecated fs.existsSync().

cjihrig · 2015-06-03T13:19:08Z

test/common.js

@@ -20,6 +20,13 @@ if (process.env.TEST_THREAD_ID) {
 }
 exports.tmpDir = path.join(exports.testDir, exports.tmpDirName);

+// Create tmpDir if it does not exist
+try {
+  fs.accessSync(exports.tmpDir);


Why not just try { fs.mkdirSync(exports.tmpDir); }? You don't need the accessSync() call at all then. And mkdirSync() being in the catch block is wrong because it can still throw.

My thinking was that if there is no temp directory and creating the temp directory failed, I wanted the CLI to blow up. I should change it to what you have and catch EEXIST and rethrow other errors, or something like that...

Fishrock123 · 2015-06-03T14:22:35Z

See: #1876 (comment)

Trott · 2015-06-08T22:56:23Z

Pushed a new commit with POC attempt number 2.

Features/concerns:

Incorporated about 50 lines of code based on rimraf to do synchronous rm -rf. (Right way to attribute? Put it in a comment?)
Code smell: Checking for the existence of process.send to determine if the process is a child process or not. (We don't want to clean out the temp directories when a test forks or spawns.) Better solution there would be most welcome.
Had to remove unnecessary require('../common') from a couple fixtures because that was causing the temp directories to be cleaned out a second time and caused the tests to fail.
Had to fix test-fs-watch-recursive.js to ignore not just its own stale watch events but also stale watch events from other tests. See Test via iojs throws but python/make do not report error #1876 (comment)
Removed temp directory creation/cleaning from the Python code so it only happens in one place.
I've only tested this on my own computer. Would be curious to see how it does (or doesn't) work across different operating systems etc.

Move creation of temporary directories for tests out of the Python harness and into common.js. This allows all tests to be run reliably outside of the Python wrapper.

Trott · 2015-06-09T04:21:06Z

Just pushed a minor commit that tweaks the proposed common.js code to make it more closely resemble the Python code it replaces.

rvagg · 2015-06-09T05:18:54Z

test/common.js

@@ -12,6 +12,61 @@ exports.tmpDirName = 'tmp';
 exports.PORT = +process.env.NODE_COMMON_PORT || 12346;
 exports.isWindows = process.platform === 'win32';

+var rimrafSync = function(p) {


can you change these to simple named functions please, assigning to variables is not idiomatic for this file

OK, new commit pushed with the function declarations changed to conform with the prevailing idiom already in the file!

Function declarations converted to idiomatic style for common.js

rvagg · 2015-06-09T13:33:30Z

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/783/

this lgtm, anyone else want to signoff?

Fishrock123 · 2015-06-09T14:44:19Z

LGTM

thefourtheye · 2015-06-10T05:04:04Z

test/common.js

+    fs.rmdirSync(p);
+  } catch (e) {
+    if (e.code === 'ENOENT')
+      return;


Anyway we are ignoring this error code. Should we really need this check?

Are you talking about the check in line 43?

Yes. That is kind of no-op in this context. Don't you think?

Yes, I agree. I modeled it on https://github.com/isaacs/rimraf/blob/db4936530293eae3372ffe757fad2f378b1261c5/rimraf.js#L317-L318. I wonder if it's a hot path in that context. Anyway, I'll take it out and push another commit.

Trott · 2015-06-10T17:03:45Z

I pushed a new commit based on observation by @thefourtheye. Can someone push this through Jenkins CI again to confirm that it doesn't trigger an edge case on a particular OS or anything? (I'm assuming I can't trigger Jenkins myself.)

Fishrock123 · 2015-06-10T17:09:31Z

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/796/

Trott · 2015-06-10T17:45:18Z

Should I worry about the win2008r2 test failures? Or is that something that sort of just happens sometimes?

Fishrock123 · 2015-06-10T17:48:18Z

They appear unrelated.

Trott · 2015-06-10T20:31:23Z

So, given that two lines got deleted, once Jenkins finishes re-running, do we need to get a couple of LGTM's from the right folks again? /cc @Fishrock123 @rvagg (Does it need to re-run to see if the win2008r2 issues are just gremlins?) I'd love to get this in so I can pile some other stuff on top of it in other PRs before heading out to reportedly low-connectivity Walker Creek Ranch...

Move creation of temporary directories for tests out of the Python harness and into common.js. This allows all tests to be run reliably outside of the Python wrapper. PR-URL: #1877 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

rvagg · 2015-06-12T00:30:20Z

Landed @ a6b8ee1, squashed into a single commit. didn't realise there were 3 until I started the process so I did the squash myself but in future @Trott maybe you could try and keep the PR to a single commit unless you're doing complex work across different concerns. It's sometimes handy to have multiple commits for review but they need to be squashed before landing so I tend to just --amend my single PR commits as I make changes and --force them up.

Thanks for the contribution and I look forward to seeing the additional improvements you have here!

Trott · 2015-06-12T01:44:21Z

Thanks @rvagg. I wasn't sure if I should squash everything every time or do one big squash after a couple of LGTMs so that people could follow along with the evolution of the changes if they wanted to. I had been doing both depending on what felt right for the particular situation. I'll try to make a point to always just squash squash squash (unless there's a super-compelling reason not to).

Fishrock123 reviewed Jun 3, 2015
View reviewed changes

brendanashworth added the test Issues and PRs related to the tests. label Jun 3, 2015

Trott force-pushed the create-temp-dir branch from d93b108 to 7830135 Compare June 3, 2015 04:47

cjihrig reviewed Jun 3, 2015
View reviewed changes

Trott force-pushed the create-temp-dir branch from 7830135 to ecd2f05 Compare June 8, 2015 22:47

Trott mentioned this pull request Jun 8, 2015

Test via iojs throws but python/make do not report error #1876

Closed

test: create temp dir in common.js

e28e98b

Move creation of temporary directories for tests out of the Python harness and into common.js. This allows all tests to be run reliably outside of the Python wrapper.

Trott force-pushed the create-temp-dir branch from ecd2f05 to e28e98b Compare June 9, 2015 04:20

rvagg reviewed Jun 9, 2015
View reviewed changes

test: refactor to conform to file idioms

fea3573

Function declarations converted to idiomatic style for common.js

rvagg mentioned this pull request Jun 10, 2015

governance: add new collaborators #V #1552

Closed

thefourtheye reviewed Jun 10, 2015
View reviewed changes

test: remove no-op

b5acb48

rvagg closed this Jun 12, 2015

This was referenced Jun 12, 2015

test: only refresh tmpDir for tests that need it #1954

Closed

test: tmpdir creation failures should fail tests #1976

Closed

Trott deleted the create-temp-dir branch October 14, 2021 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: create temp dir in common.js #1877

test: create temp dir in common.js #1877

Trott commented Jun 3, 2015

Fishrock123 Jun 3, 2015

mscdex Jun 3, 2015

Trott Jun 3, 2015

mscdex Jun 3, 2015

Trott commented Jun 3, 2015

cjihrig Jun 3, 2015

Trott Jun 3, 2015

Fishrock123 commented Jun 3, 2015

Trott commented Jun 8, 2015

Trott commented Jun 9, 2015

rvagg Jun 9, 2015

Trott Jun 9, 2015

rvagg commented Jun 9, 2015

Fishrock123 commented Jun 9, 2015

thefourtheye Jun 10, 2015

Trott Jun 10, 2015

thefourtheye Jun 10, 2015

Trott Jun 10, 2015

Trott commented Jun 10, 2015

Fishrock123 commented Jun 10, 2015

Trott commented Jun 10, 2015

Fishrock123 commented Jun 10, 2015

Trott commented Jun 10, 2015

rvagg commented Jun 12, 2015

Trott commented Jun 12, 2015

test: create temp dir in common.js #1877

test: create temp dir in common.js #1877

Conversation

Trott commented Jun 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Jun 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Jun 3, 2015

Trott commented Jun 8, 2015

Trott commented Jun 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Jun 9, 2015

Fishrock123 commented Jun 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Jun 10, 2015

Fishrock123 commented Jun 10, 2015

Trott commented Jun 10, 2015

Fishrock123 commented Jun 10, 2015

Trott commented Jun 10, 2015

rvagg commented Jun 12, 2015

Trott commented Jun 12, 2015