From 3cd947981362261d053547a4dde5e9559f4471f3 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 13 Nov 2020 15:00:13 -0800 Subject: [PATCH] Run fixture cleanup asynchronously, after teardown This makes the fixture cleanup _always_ run after any user-supplied teardown functions (which was ambiguous before), and removes using the async form of `rimraf` so that it can retry with backoff if the directories are locked. Note: in libtap, this requires adding an async recursive removal function to settings. PR-URL: https://github.com/tapjs/node-tap/pull/704 --- lib/test.js | 18 ++++++++++++++++-- settings.js | 21 ++++++++++++++++++++- tap-snapshots/test-settings.js-TAP.test.cjs | 1 + test/clean-stacks.js | 2 +- test/fixture.js | 1 + test/settings.js | 12 ++++++++++++ test/snapshot.js | 1 + test/test.js | 16 ++++++++++++++-- 8 files changed, 66 insertions(+), 6 deletions(-) diff --git a/lib/test.js b/lib/test.js index d8cdb818..4c831c79 100644 --- a/lib/test.js +++ b/lib/test.js @@ -61,6 +61,7 @@ const _beforeEnd = Symbol('_beforeEnd') const _emits = Symbol('_emits') const _nextChildId = Symbol('_nextChildId') const _expectUncaught = Symbol('_expectUncaught') +const _createdFixture = Symbol('_createdFixture') const hasOwn = (obj, key) => Object.prototype.hasOwnProperty.call(obj, key) @@ -90,6 +91,7 @@ class Test extends Base { this.doingStdinOnly = false this.onTeardown = [] + this[_createdFixture] = false this.subtests = [] this.pool = new Pool() this.queue = ['TAP version 13\n'] @@ -483,6 +485,7 @@ class Test extends Base { } emitSubTeardown (p) { + // if it's not a thing that CAN have teardowns, nothing to do here if (!p.onTeardown) return @@ -514,7 +517,18 @@ class Test extends Base { } } - p.emit('teardown') + // ok we're done, just delete the fixture if it created one. + // do this AFTER all user-generated teardowns, and asynchronously so + // that we can do the fancy backoff dance for Win32's weirdo fs. + if (p[_createdFixture]) { + const {rmdirRecursive} = settings + return new Promise((res, rej) => { + rmdirRecursive(p[_createdFixture], er => + er ? /* istanbul ignore next - rimraf never fails lol */ rej(er) + : res()) + }).then(() => p.emit('teardown')) + } else + p.emit('teardown') } writeSubComment (p, cb) { @@ -1166,7 +1180,7 @@ class Test extends Base { const dir = this.testdirName rmdirRecursiveSync(dir) if (!this.saveFixture) - this.teardown(() => rmdirRecursiveSync(dir)) + this[_createdFixture] = dir Fixture.make(dir, fixture || {}) return dir } diff --git a/settings.js b/settings.js index b981647e..1f2516df 100644 --- a/settings.js +++ b/settings.js @@ -5,6 +5,7 @@ const StackUtils = require('stack-utils') // Just unconditionally use fs.rmdirSync after LTS/12 is required let rmdirRecursiveSync +let rmdirRecursive module.exports = { atTap: false, @@ -13,6 +14,23 @@ module.exports = { /* istanbul ignore next: version specific */ return !rmdirRecursiveSync && (nodeMajor < 12 || (nodeMajor === 12 && nodeMinor < 10)) }, + get rmdirRecursive() { + /* istanbul ignore next: version specific */ + if (!rmdirRecursive) { + return () => { + throw new Error("require('libtap/settings').rmdirRecursive must be initialized for Node.js <12.10.0") + } + } + + return rmdirRecursive + }, + set rmdirRecursive(value) { + if (typeof value !== 'function' || value.length !== 2) { + throw new TypeError('rmdirRecursive must be a function with exactly two arguments') + } + + rmdirRecursive = value + }, get rmdirRecursiveSync() { /* istanbul ignore next: version specific */ if (!rmdirRecursiveSync) { @@ -42,5 +60,6 @@ module.exports = { /* istanbul ignore next: version specific */ if (!module.exports.rimrafNeeded) { const fs = require('fs') - rmdirRecursiveSync = dir => fs.rmdirSync(dir, {recursive: true}) + rmdirRecursiveSync = dir => fs.rmdirSync(dir, {recursive: true, force: true}) + rmdirRecursive = (dir, cb) => fs.rmdir(dir, {recursive: true, force: true}, cb) } diff --git a/tap-snapshots/test-settings.js-TAP.test.cjs b/tap-snapshots/test-settings.js-TAP.test.cjs index b1f1ff83..7b07a9f1 100644 --- a/tap-snapshots/test-settings.js-TAP.test.cjs +++ b/tap-snapshots/test-settings.js-TAP.test.cjs @@ -10,6 +10,7 @@ Object { "atTap": false, "output": "process.stdout", "rimrafNeeded": "version specific", + "rmdirRecursive": Function rmdirRecursive(dir, cb), "rmdirRecursiveSync": Function rmdirRecursiveSync(dir), "stackUtils": Object { "ignoredPackages": Array [], diff --git a/test/clean-stacks.js b/test/clean-stacks.js index f3c0e6ad..5ef6fb45 100644 --- a/test/clean-stacks.js +++ b/test/clean-stacks.js @@ -55,7 +55,7 @@ module.exports = out => out .replace(/\n( +)method: .*(\n\1 .*)*\n/g, '\n') .replace(/\n( +)type: .*\n/g, '\n') .replace(/\n( +)file: (.*)\n/g, ($0, $1, $2) => - internals.indexOf($2.replace(/\.js$/, '')) === -1 ? $0 + internals.indexOf($2.replace(/\.js$/, '')) === -1 && !/node:/.test($2) ? $0 : '\n' + $1 + 'file: #INTERNAL#\n' ) diff --git a/test/fixture.js b/test/fixture.js index 070e1a5b..e94bb9af 100644 --- a/test/fixture.js +++ b/test/fixture.js @@ -6,6 +6,7 @@ const dir = t.testdirName if (settings.rimrafNeeded) { settings.rmdirRecursiveSync = dir => require('rimraf').sync(dir, {glob: false}) + settings.rmdirRecursive = (dir, cb) => require('rimraf')(dir, {glob: false}, cb) } settings.rmdirRecursiveSync(dir) diff --git a/test/settings.js b/test/settings.js index dee16803..33124bb1 100644 --- a/test/settings.js +++ b/test/settings.js @@ -11,6 +11,7 @@ t.matchSnapshot({ ...settings, rimrafNeeded: 'version specific', rmdirRecursiveSync(dir) {}, + rmdirRecursive(dir, cb) {}, output: 'process.stdout', stackUtils: { ...settings.stackUtils, @@ -26,7 +27,18 @@ t.throws(_ => { settings.rmdirRecursiveSync = () => {} }, TypeError) +t.throws(_ => { + settings.rmdirRecursive = 'this is not a function' +}, TypeError) + +t.throws(_ => { + settings.rmdirRecursive = () => {} +}, TypeError) + const replacement = dir => {} +const replacementAsync = (dir, cb) => {} settings.rmdirRecursiveSync = replacement +settings.rmdirRecursive = replacementAsync t.equal(settings.rimrafNeeded, false) t.equal(settings.rmdirRecursiveSync, replacement) +t.equal(settings.rmdirRecursive, replacementAsync) diff --git a/test/snapshot.js b/test/snapshot.js index 665cae8c..52810444 100644 --- a/test/snapshot.js +++ b/test/snapshot.js @@ -8,6 +8,7 @@ const fs = require('fs') if (settings.rimrafNeeded) { settings.rmdirRecursiveSync = dir => require('rimraf').sync(dir, {glob: false}) + settings.rmdirRecursive = (dir, cb) => require('rimraf')(dir, {glob: false}, cb) } t.test('cleanup first', t => { diff --git a/test/test.js b/test/test.js index bf52b79d..08f6fd1d 100644 --- a/test/test.js +++ b/test/test.js @@ -11,6 +11,7 @@ const settings = require('../settings.js') if (settings.rimrafNeeded) { settings.rmdirRecursiveSync = dir => require('rimraf').sync(dir, {glob: false}) + settings.rmdirRecursive = (dir, cb) => require('rimraf')(dir, {glob: false}, cb) } // set this forcibly so it doesn't interfere with other tests. @@ -1200,10 +1201,12 @@ t.test('test dir name does not throw when no main module is present', t => { }) }) -t.test('save a fixture', t => { +t.test('fixture dir stuff', t => { const tdn = t.testdirName t.throws(() => fs.statSync(tdn), 'doesnt exist yet') + t.teardown(() => fs.statSync(tdn), 'exists in teardown') const dir = t.testdir() + t.teardown(() => fs.statSync(tdn), 'exists in teardown scheduled after testdir') t.equal(dir, tdn) t.ok(fs.statSync(dir).isDirectory(), 'made directory') t.testdir({ file: 'contents' }) @@ -1215,12 +1218,21 @@ t.test('save a fixture', t => { t.throws(() => fs.statSync(`${dir}/file`), 'old dir cleared out') t.equal(fs.readFileSync(`${dir}/file2`, 'utf8'), 'contents', 'made file') t.equal(fs.readlinkSync(`${dir}/link`), 'file2', 'made symlink') + let removeDir + t.test('remove the dir when its done', t => { + removeDir = t.testdir() + t.end() + }) let leaveDir t.test('leave the dir behind', { saveFixture: true }, t => { + t.throws(() => fs.statSync(removeDir), 'previous dir was removed') leaveDir = t.testdir() t.parent.teardown(() => settings.rmdirRecursiveSync(leaveDir)) t.end() }) - t.ok(fs.statSync(leaveDir).isDirectory(), 'left dir behind') + t.test('check leaveDir is still there', t => { + t.ok(fs.statSync(leaveDir).isDirectory(), 'left dir behind') + t.end() + }) t.end() })