Skip to content

Commit 8b1806a

Browse files
silkentrancesilkentrance
committed
fix #210: regression after bug fix for #197 
1 parent be6fce9 commit 8b1806a

File tree

4 files changed

+62
-42
lines changed

4 files changed

+62
-42
lines changed

‎lib/tmp.js

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,10 @@ const
4242
SIGINT = 'SIGINT',
4343

4444
// this will hold the objects need to be removed on exit
45-
_removeObjects = [];
45+
_removeObjects = [],
46+
47+
// API change in fs.rmdirSync leads to error when passing in a second parameter, e.g. the callback
48+
FN_RMDIR_SYNC = fs.rmdirSync.bind(fs);
4649

4750
var
4851
_gracefulCleanup = false;
@@ -253,7 +256,7 @@ function file(options, callback) {
253256

254257
// FIXME overall handling of opts.discardDescriptor is off
255258
if (opts.discardDescriptor) {
256-
// FIXME must not unlink
259+
// FIXME? must not unlink as the user expects the filename to be reserved
257260
return fs.close(fd, function _discardCallback(err) {
258261
/* istanbul ignore else */
259262
if (err) {
@@ -464,7 +467,7 @@ function _rimrafRemoveDirSyncWrapper(dirPath, next) {
464467
*/
465468
function _prepareTmpDirRemoveCallback(name, opts, sync) {
466469
const removeFunction = opts.unsafeCleanup ? _rimrafRemoveDirWrapper : fs.rmdir.bind(fs);
467-
const removeFunctionSync = opts.unsafeCleanup ? _rimrafRemoveDirSyncWrapper : fs.rmdirSync.bind(fs);
470+
const removeFunctionSync = opts.unsafeCleanup ? _rimrafRemoveDirSyncWrapper : FN_RMDIR_SYNC;
468471
const removeCallbackSync = _prepareRemoveCallback(removeFunctionSync, name, sync);
469472
const removeCallback = _prepareRemoveCallback(removeFunction, name, sync, removeCallbackSync);
470473
if (!opts.keep) _removeObjects.unshift(removeCallbackSync);
@@ -500,7 +503,7 @@ function _prepareRemoveCallback(removeFunction, fileOrDirName, sync, cleanupCall
500503
if (index >= 0) _removeObjects.splice(index, 1);
501504

502505
called = true;
503-
if (sync) {
506+
if (sync || removeFunction === FN_RMDIR_SYNC) {
504507
return removeFunction(fileOrDirName);
505508
} else {
506509
return removeFunction(fileOrDirName, next || function() {});

‎test/dir-sync-test.js

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
var
55
assert = require('assert'),
6-
fs = require('fs'),
76
path = require('path'),
87
inbandStandardTests = require('./inband-standard'),
98
childProcess = require('./child-process').genericChildProcess,
@@ -20,7 +19,7 @@ describe('tmp', function () {
2019
describe('when running inband standard tests', function () {
2120
inbandStandardTests(false, function before() {
2221
this.topic = tmp.dirSync(this.opts);
23-
});
22+
}, true);
2423

2524
describe('with invalid tries', function () {
2625
it('should result in an error on negative tries', function () {
@@ -68,8 +67,8 @@ describe('tmp', function () {
6867
if (!stderr) return done(new Error('stderr expected'));
6968
try {
7069
assertions.assertExists(stdout);
71-
} catch (err) {
7270
rimraf.sync(stdout);
71+
} catch (err) {
7372
return done(err);
7473
}
7574
done();
@@ -82,8 +81,8 @@ describe('tmp', function () {
8281
if (stderr) return done(new Error(stderr));
8382
try {
8483
assertions.assertExists(stdout);
85-
} catch (err) {
8684
rimraf.sync(stdout);
85+
} catch (err) {
8786
return done(err);
8887
}
8988
done();
@@ -130,8 +129,8 @@ describe('tmp', function () {
130129
} else {
131130
assertions.assertExists(path.join(stdout, 'symlinkme-target'));
132131
}
133-
} catch (err) {
134132
rimraf.sync(stdout);
133+
} catch (err) {
135134
return done(err);
136135
}
137136
done();

‎test/file-sync-test.js

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
var
55
assert = require('assert'),
6-
fs = require('fs'),
76
inbandStandardTests = require('./inband-standard'),
87
assertions = require('./assertions'),
98
childProcess = require('./child-process').genericChildProcess,
@@ -20,7 +19,7 @@ describe('tmp', function () {
2019
describe('when running inband standard tests', function () {
2120
inbandStandardTests(true, function before() {
2221
this.topic = tmp.fileSync(this.opts);
23-
});
22+
}, true);
2423

2524
describe('with invalid tries', function () {
2625
it('should result in an error on negative tries', function () {
@@ -71,8 +70,8 @@ describe('tmp', function () {
7170
if (!stderr) return done(new Error('stderr expected'));
7271
try {
7372
assertions.assertExists(stdout, true);
74-
} catch (err) {
7573
rimraf.sync(stdout);
74+
} catch (err) {
7675
return done(err);
7776
}
7877
done();
@@ -85,8 +84,8 @@ describe('tmp', function () {
8584
if (stderr) return done(new Error(stderr));
8685
try {
8786
assertions.assertExists(stdout, true);
88-
} catch (err) {
8987
rimraf.sync(stdout);
88+
} catch (err) {
9089
return done(err);
9190
}
9291
done();
@@ -122,6 +121,19 @@ describe('tmp', function () {
122121
done();
123122
});
124123
});
124+
it('on issue #115', function (done) {
125+
childProcess(this, 'issue115-sync.json', function (err, stderr, stdout) {
126+
if (err) return done(err);
127+
if (stderr) return done(new Error(stderr));
128+
try {
129+
assertions.assertDoesNotExist(stdout);
130+
} catch (err) {
131+
rimraf.sync(stdout);
132+
return done(err);
133+
}
134+
done();
135+
});
136+
});
125137
});
126138
});
127139
});

‎test/inband-standard.js

Lines changed: 35 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -3,32 +3,30 @@
33

44
var
55
assert = require('assert'),
6-
fs = require('fs'),
76
path = require('path'),
8-
existsSync = fs.existsSync || path.existsSync,
97
assertions = require('./assertions'),
108
rimraf = require('rimraf'),
119
tmp = require('../lib/tmp');
1210

1311

14-
module.exports = function inbandStandard(isFile, beforeHook) {
15-
var testMode = isFile ? 0600 : 0700;
16-
describe('without any parameters', inbandStandardTests({ mode: testMode, prefix: 'tmp-' }, null, isFile, beforeHook));
17-
describe('with prefix', inbandStandardTests({ mode: testMode }, { prefix: 'something' }, isFile, beforeHook));
18-
describe('with postfix', inbandStandardTests({ mode: testMode }, { postfix: '.txt' }, isFile, beforeHook));
19-
describe('with template and no leading path', inbandStandardTests({ mode: testMode, prefix: 'clike-', postfix: '-postfix' }, { template: 'clike-XXXXXX-postfix' }, isFile, beforeHook));
20-
describe('with template and leading path', inbandStandardTests({ mode: testMode, prefix: 'clike-', postfix: '-postfix' }, { template: path.join(tmp.tmpdir, 'clike-XXXXXX-postfix')}, isFile, beforeHook));
21-
describe('with name', inbandStandardTests({ mode: testMode }, { name: 'using-name' }, isFile, beforeHook));
22-
describe('with mode', inbandStandardTests(null, { mode: 0755 }, isFile, beforeHook));
23-
describe('with multiple options', inbandStandardTests(null, { prefix: 'foo', postfix: 'bar', mode: 0750 }, isFile, beforeHook));
12+
module.exports = function inbandStandard(isFile, beforeHook, sync = false) {
13+
var testMode = isFile ? 0o600 : 0o700;
14+
describe('without any parameters', inbandStandardTests({ mode: testMode, prefix: 'tmp-' }, null, isFile, beforeHook, sync));
15+
describe('with prefix', inbandStandardTests({ mode: testMode, prefix: 'tmp-' }, { prefix: 'tmp-something' }, isFile, beforeHook, sync));
16+
describe('with postfix', inbandStandardTests({ mode: testMode, prefix: 'tmp-' }, { postfix: '.txt' }, isFile, beforeHook, sync));
17+
describe('with template and no leading path', inbandStandardTests({ mode: testMode, prefix: 'tmp-clike-', postfix: '-postfix' }, { template: 'tmp-clike-XXXXXX-postfix' }, isFile, beforeHook, sync));
18+
describe('with template and leading path', inbandStandardTests({ mode: testMode, prefix: 'tmp-clike-', postfix: '-postfix' }, { template: path.join(tmp.tmpdir, 'tmp-clike-XXXXXX-postfix')}, isFile, beforeHook, sync));
19+
describe('with name', inbandStandardTests({ mode: testMode }, { name: 'tmp-using-name' }, isFile, beforeHook, sync));
20+
describe('with mode', inbandStandardTests(null, { mode: 0o755 }, isFile, beforeHook, sync));
21+
describe('with multiple options', inbandStandardTests(null, { prefix: 'tmp-multiple', postfix: 'bar', mode: 0o750 }, isFile, beforeHook, sync));
2422
if (isFile) {
25-
describe('with discardDescriptor', inbandStandardTests(null, { mode: testMode, discardDescriptor: true }, isFile, beforeHook));
26-
describe('with detachDescriptor', inbandStandardTests(null, { mode: testMode, detachDescriptor: true }, isFile, beforeHook));
23+
describe('with discardDescriptor', inbandStandardTests(null, { mode: testMode, discardDescriptor: true }, isFile, beforeHook, sync));
24+
describe('with detachDescriptor', inbandStandardTests(null, { mode: testMode, detachDescriptor: true }, isFile, beforeHook, sync));
2725
}
2826
};
2927

3028

31-
function inbandStandardTests(testOpts, opts, isFile, beforeHook) {
29+
function inbandStandardTests(testOpts, opts, isFile, beforeHook, sync = false) {
3230
return function () {
3331
opts = opts || {};
3432
testOpts = testOpts || {};
@@ -51,7 +49,7 @@ function inbandStandardTests(testOpts, opts, isFile, beforeHook) {
5149
assertions.assertMode(this.topic.name, testOpts.mode || opts.mode);
5250
}.bind(topic));
5351

54-
if (opts.prefix || testOpts.prefix) {
52+
if(testOpts.prefix || opts.prefix) {
5553
it('should have the expected prefix', function () {
5654
assertions.assertPrefix(this.topic.name, testOpts.prefix || opts.prefix);
5755
}.bind(topic));
@@ -73,18 +71,26 @@ function inbandStandardTests(testOpts, opts, isFile, beforeHook) {
7371
}.bind(topic));
7472
}
7573

76-
it('should have a working removeCallback', function (done) {
77-
const self = this;
78-
this.topic.removeCallback(function (err) {
79-
if (err) return done(err);
80-
try {
81-
assertions.assertDoesNotExist(self.topic.name);
82-
} catch (err) {
83-
rimraf.sync(self.topic.name);
84-
return done(err);
85-
}
86-
done();
87-
});
88-
}.bind(topic));
74+
if (sync) {
75+
it('should have a working removeCallback', function () {
76+
assert.ok(typeof this.topic.removeCallback === 'function');
77+
// important: remove file or dir unconditionally
78+
rimraf.sync(this.topic.name);
79+
}.bind(topic));
80+
} else {
81+
it('should have a working removeCallback', function (done) {
82+
const self = this;
83+
this.topic.removeCallback(function (err) {
84+
if (err) return done(err);
85+
try {
86+
assertions.assertDoesNotExist(self.topic.name);
87+
} catch (err) {
88+
rimraf.sync(self.topic.name);
89+
return done(err);
90+
}
91+
done();
92+
});
93+
}.bind(topic));
94+
}
8995
};
9096
}

0 commit comments

Comments
 (0)