Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: create temp dir in common.js #1877

Closed
wants to merge 3 commits into from
Closed

test: create temp dir in common.js #1877

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e28e98b
test: create temp dir in common.js
Trott Jun 3, 2015
fea3573
test: refactor to conform to file idioms
Trott Jun 9, 2015
b5acb48
test: remove no-op
Trott Jun 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions test/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,61 @@ exports.tmpDirName = 'tmp';
exports.PORT = +process.env.NODE_COMMON_PORT || 12346;
exports.isWindows = process.platform === 'win32';

function rimrafSync(p) {
try {
var st = fs.lstatSync(p);
} catch (e) {
if (e.code === 'ENOENT')
return;
}

try {
if (st && st.isDirectory())
rmdirSync(p, null);
else
fs.unlinkSync(p);
} catch (e) {
if (e.code === 'ENOENT')
return;
if (e.code === 'EPERM')
return rmdirSync(p, er);
if (e.code !== 'EISDIR')
throw e;
rmdirSync(p, e);
}
}

function rmdirSync(p, originalEr) {
try {
fs.rmdirSync(p);
} catch (e) {
if (e.code === 'ENOENT')
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about the check in line 43?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (e.code === 'ENOTDIR')
throw originalEr;
if (e.code === 'ENOTEMPTY' || e.code === 'EEXIST' || e.code === 'EPERM') {
fs.readdirSync(p).forEach(function(f) {
rimrafSync(path.join(p, f));
});
fs.rmdirSync(p);
}
}
}

function refreshTmpDir() {
if (!process.send) { // Not a child process
try {
rimrafSync(exports.tmpDir);
} catch (e) {
}

try {
fs.mkdirSync(exports.tmpDir);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather this one be unguarded, if the mkdir fails it should fail the test I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I was trying to keep this change as small as possible. The current Python script swallows errors if mkdir fails, so I did the same. (Actually, it swallows errors and keeps retrying if the directory doesn't exist. Comment says that's because Python mkdir on win32 fails intermittently. I was going to wait and see if that actually crops up as an issue with fs.mkdirSync before duplicating that functionality.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to agree with Rod. If this fails a bunch of tests will (should?) fail anyways.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Replacing comment I just deleted because, uh, wrong topic. Let's try again.) Can we do that as a separate PR? As with the expose-refresh-and-put-in-individual-tests, I'm happy to do it and even excited to do it because it's The Right Thing to do. But I want to keep the scope of the individual change sets as narrow and manageable as possible. If that change does break something when jenkins is run, I wouldn't want that change to hold up the rest of the stuff that's here.

} catch (e) {
}
}
}

if (process.env.TEST_THREAD_ID) {
// Distribute ports in parallel tests
if (!process.env.NODE_COMMON_PORT)
Expand All @@ -21,6 +76,8 @@ if (process.env.TEST_THREAD_ID) {
}
exports.tmpDir = path.join(exports.testDir, exports.tmpDirName);

refreshTmpDir();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my preference would be to make this callable by the individual tests rather than happening for each test, we're trying to speed up tests and this will just slow them down

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually - that comment probably isn't fair without actually benchmarking this, so I'll reserve judgement until someone does

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current Python script deletes and creates the temporary directory for each and every test. This duplicates that functionality. Making it something the individual tests call might be a decent performance win but it would also be a more invasive change, touching a lot of tests. I'd rather do that as two separate PRs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I've got this (tmp dir refreshing handled by individual tests so it only happens for tests that use the tmp dir) ready to go once this PR goes through.


var opensslCli = null;
var inFreeBSDJail = null;
var localhostIPv4 = null;
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/print-chars-from-buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
var common = require('../common');
var assert = require('assert');

var n = parseInt(process.argv[2]);
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/print-chars.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
var common = require('../common');
var assert = require('assert');

var n = parseInt(process.argv[2]);
Expand Down
6 changes: 2 additions & 4 deletions test/sequential/test-fs-watch-recursive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,10 @@ if (process.platform === 'darwin') {
watcher.on('change', function(event, filename) {
assert.ok('change' === event || 'rename' === event);

// Ignore stale events generated by mkdir
if (filename === testsubdirName)
// Ignore stale events generated by mkdir and other tests
if (filename !== relativePathOne)
return;

assert.equal(relativePathOne, filename);

watcher.close();
cleanup();
++watchSeenOne;
Expand Down
30 changes: 0 additions & 30 deletions test/testpy/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import test
import os
import shutil
from shutil import rmtree
from os import mkdir
from glob import glob
from os.path import join, dirname, exists
Expand All @@ -50,35 +49,6 @@ def __init__(self, path, file, arch, mode, context, config, additional=[]):
self.tmpdir = join(dirname(self.config.root), 'tmp')
self.additional_flags = additional

def GetTmpDir(self):
return "%s.%d" % (self.tmpdir, self.thread_id)


def AfterRun(self, result):
# delete the whole tmp dir
try:
rmtree(self.GetTmpDir())
except:
pass
# make it again.
try:
mkdir(self.GetTmpDir())
except:
pass

def BeforeRun(self):
# delete the whole tmp dir
try:
rmtree(self.GetTmpDir())
except:
pass
# make it again.
# intermittently fails on win32, so keep trying
while not os.path.exists(self.GetTmpDir()):
try:
mkdir(self.GetTmpDir())
except:
pass

def GetLabel(self):
return "%s %s" % (self.mode, self.GetName())
Expand Down