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: tapjs/tapjs#704

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
   }

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)
 }

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 [],

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'
   )
 

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)

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)

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 => {

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()
 })