test: Clean up some flakiness and inconsistency

* Get rid of a lot of usage of osenv.tmpdir in tests.
* Remove unnecessary creation/deletion of common.pkg
* Reduce rimraf.sync wherever possible (often collides with
  common-tap.js's rimraf on windows)
* Use common test utilities wherever possible.
* DRY tests with test templates where it makes sense.

PR-URL: #240
Credit: @isaacs
Close: #240
Reviewed-by: @ruyadorno

Author: isaacs

test/common-tap.js

@@ -216,17 +216,15 @@ exports.readBinLink = function (path) {
 
 exports.skipIfWindows = function (why) {
   if (!isWindows) return
-  console.log('1..1')
   if (!why) why = 'this test not available on windows'
-  console.log('ok 1 # skip ' + why)
+  require('tap').plan(0, why)
   process.exit(0)
 }
 
 exports.pendIfWindows = function (why) {
   if (!isWindows) return
-  console.log('1..1')
   if (!why) why = 'this test is pending further changes on windows'
-  console.log('not ok 1 # todo ' + why)
+  require('tap').fail(' ', { todo: why, diagnostic: false })
   process.exit(0)
 }
 

test/tap/404-parent.js

@@ -1,7 +1,6 @@
 var common = require('../common-tap.js')
 var test = require('tap').test
 var npm = require('../../')
-var osenv = require('osenv')
 var path = require('path')
 var fs = require('fs')
 var rimraf = require('rimraf')
@@ -10,20 +9,15 @@ var mr = require('npm-registry-mock')
 
 test('404-parent: if parent exists, specify parent in error message', function (t) {
   setup()
-  rimraf.sync(path.resolve(pkg, 'node_modules'))
-  performInstall(function (err) {
-    t.ok(err instanceof Error, 'error was returned')
-    t.equal(err.parent, '404-parent', "error's parent set")
-    t.end()
+  rimraf(path.resolve(pkg, 'node_modules'), () => {
+    performInstall(function (err) {
+      t.ok(err instanceof Error, 'error was returned')
+      t.equal(err.parent, '404-parent', "error's parent set")
+      t.end()
+    })
   })
 })
 
-test('cleanup', function (t) {
-  process.chdir(osenv.tmpdir())
-  rimraf.sync(pkg)
-  t.end()
-})
-
 function setup () {
   fs.writeFileSync(path.resolve(pkg, 'package.json'), JSON.stringify({
     author: 'Evan Lucas',

test/tap/access.js

@@ -73,8 +73,7 @@ test('npm access public when no package passed and no package.json', function (t
   function (er, code, stdout, stderr) {
     t.ifError(er, 'npm access')
     t.match(stderr, /no package name passed to command and no package.json found/)
-    rimraf.sync(missing)
-    t.end()
+    rimraf(missing, t.end)
   })
 })
 
@@ -95,8 +94,7 @@ test('npm access public when no package passed and invalid package.json', functi
   function (er, code, stdout, stderr) {
     t.ifError(er, 'npm access')
     t.match(stderr, /Failed to parse json/)
-    rimraf.sync(invalid)
-    t.end()
+    rimraf(invalid, t.end)
   })
 })
 
@@ -405,8 +403,7 @@ test('npm access ls-packages with no package specified or package.json', functio
     function (er, code, stdout, stderr) {
       t.ifError(er, 'npm access ls-packages')
       t.same(JSON.parse(stdout), clientPackages)
-      rimraf.sync(missing)
-      t.end()
+      rimraf(missing, t.end)
     }
   )
 })
@@ -557,7 +554,6 @@ test('npm access blerg', function (t) {
 
 test('cleanup', function (t) {
   t.pass('cleaned up')
-  rimraf.sync(pkg)
   server.done()
   server.close()
   t.end()

test/tap/add-remote-git-file.js

@@ -4,17 +4,16 @@ var fs = require('fs')
 var resolve = require('path').resolve
 var url = require('url')
 
-var osenv = require('osenv')
 var mkdirp = require('mkdirp')
-var rimraf = require('rimraf')
 var test = require('tap').test
 
 var npm = require('../../lib/npm.js')
 var fetchPackageMetadata = require('../../lib/fetch-package-metadata.js')
 var common = require('../common-tap.js')
 
-var pkg = common.pkg
-var repo = common.pkg + '-repo'
+var pkg = resolve(common.pkg, 'package')
+var repo = resolve(common.pkg, 'repo')
+mkdirp.sync(pkg)
 
 var git
 var cloneURL = 'git+file://' + resolve(pkg, 'child.git')
@@ -25,7 +24,6 @@ var pjChild = JSON.stringify({
 }, null, 2) + '\n'
 
 test('setup', function (t) {
-  bootstrap()
   setup(function (er, r) {
     t.ifError(er, 'git started up successfully')
 
@@ -70,16 +68,6 @@ test('save install', function (t) {
   })
 })
 
-test('clean', function (t) {
-  cleanup()
-  t.end()
-})
-
-function bootstrap () {
-  cleanup()
-  mkdirp.sync(pkg)
-}
-
 function setup (cb) {
   mkdirp.sync(repo)
   fs.writeFileSync(resolve(repo, 'package.json'), pjChild)
@@ -95,9 +83,3 @@ function setup (cb) {
     }, cb)
   })
 }
-
-function cleanup () {
-  process.chdir(osenv.tmpdir())
-  rimraf.sync(repo)
-  rimraf.sync(pkg)
-}

test/tap/add-remote-git-shrinkwrap.js

@@ -1,16 +1,14 @@
 var fs = require('fs')
 var resolve = require('path').resolve
 
-var osenv = require('osenv')
 var mkdirp = require('mkdirp')
-var rimraf = require('rimraf')
 var test = require('tap').test
 
 var npm = require('../../lib/npm.js')
 var common = require('../common-tap.js')
 
-var pkg = common.pkg
-var repo = pkg + '-repo'
+var pkg = resolve(common.pkg, 'package')
+var repo = resolve(common.pkg, 'repo')
 
 var daemon
 var daemonPID
@@ -30,7 +28,8 @@ var pjChild = JSON.stringify({
 }, null, 2) + '\n'
 
 test('setup', function (t) {
-  bootstrap()
+  mkdirp.sync(pkg)
+  fs.writeFileSync(resolve(pkg, 'package.json'), pjParent)
   setup(function (er, r) {
     t.ifError(er, 'git started up successfully')
 
@@ -85,19 +84,10 @@ test('shrinkwrap gets correct _from and _resolved (#7121)', function (t) {
 })
 
 test('clean', function (t) {
-  daemon.on('close', function () {
-    cleanup()
-    t.end()
-  })
+  daemon.on('close', t.end)
   process.kill(daemonPID)
 })
 
-function bootstrap () {
-  cleanup()
-  mkdirp.sync(pkg)
-  fs.writeFileSync(resolve(pkg, 'package.json'), pjParent)
-}
-
 function setup (cb) {
   mkdirp.sync(repo)
   fs.writeFileSync(resolve(repo, 'package.json'), pjChild)
@@ -145,9 +135,3 @@ function setup (cb) {
     }, cb)
   })
 }
-
-function cleanup () {
-  process.chdir(osenv.tmpdir())
-  rimraf.sync(repo)
-  rimraf.sync(pkg)
-}

test/tap/add-remote-git-submodule.js

@@ -1,16 +1,16 @@
 var fs = require('fs')
 var resolve = require('path').resolve
 
-var osenv = require('osenv')
+var cwd = process.cwd()
 var mkdirp = require('mkdirp')
 var rimraf = require('rimraf')
 var test = require('tap').test
 
 var npm = require('../../lib/npm.js')
 var common = require('../common-tap.js')
 
-var pkg = common.pkg
-var repos = pkg + '-repos'
+var pkg = resolve(common.pkg, 'package')
+var repos = resolve(common.pkg, 'repos')
 var subwt = resolve(repos, 'subwt')
 var topwt = resolve(repos, 'topwt')
 var suburl = 'git://localhost:' + common.gitPort + '/sub.git'
@@ -62,14 +62,13 @@ test('has file in submodule', function (t) {
 
 test('clean', function (t) {
   daemon.on('close', function () {
-    cleanup()
     t.end()
   })
   process.kill(daemonPID)
 })
 
 function bootstrap (t) {
-  process.chdir(osenv.tmpdir())
+  process.chdir(cwd)
   rimraf.sync(pkg)
   mkdirp.sync(pkg)
   process.chdir(pkg)
@@ -141,9 +140,3 @@ function setup (cb) {
     }, cb)
   })
 }
-
-function cleanup () {
-  process.chdir(osenv.tmpdir())
-  rimraf.sync(repos)
-  rimraf.sync(pkg)
-}

test/tap/add-remote-git.js

@@ -1,16 +1,14 @@
 var fs = require('fs')
 var resolve = require('path').resolve
 
-var osenv = require('osenv')
 var mkdirp = require('mkdirp')
-var rimraf = require('rimraf')
 var test = require('tap').test
 
 var npm = require('../../lib/npm.js')
 var common = require('../common-tap.js')
 
-var pkg = common.pkg
-var repo = pkg + '-repo'
+var pkg = resolve(common.pkg, 'package')
+var repo = resolve(pkg, 'repo')
 
 var daemon
 var daemonPID
@@ -30,7 +28,8 @@ var pjChild = JSON.stringify({
 }, null, 2) + '\n'
 
 test('setup', function (t) {
-  bootstrap()
+  mkdirp.sync(pkg)
+  fs.writeFileSync(resolve(pkg, 'package.json'), pjParent)
   setup(function (er, r) {
     t.ifError(er, 'git started up successfully')
 
@@ -47,25 +46,15 @@ test('install from repo', function (t) {
   process.chdir(pkg)
   npm.commands.install('.', [], function (er) {
     t.ifError(er, 'npm installed via git')
-
     t.end()
   })
 })
 
 test('clean', function (t) {
-  daemon.on('close', function () {
-    cleanup()
-    t.end()
-  })
+  daemon.on('close', t.end)
   process.kill(daemonPID)
 })
 
-function bootstrap () {
-  cleanup()
-  mkdirp.sync(pkg)
-  fs.writeFileSync(resolve(pkg, 'package.json'), pjParent)
-}
-
 function setup (cb) {
   mkdirp.sync(repo)
   fs.writeFileSync(resolve(repo, 'package.json'), pjChild)
@@ -113,9 +102,3 @@ function setup (cb) {
     }, cb)
   })
 }
-
-function cleanup () {
-  process.chdir(osenv.tmpdir())
-  rimraf.sync(repo)
-  rimraf.sync(pkg)
-}

test/tap/all-package-metadata.js

@@ -26,8 +26,8 @@ function setup () {
   mkdirp.sync(cacheBase)
 }
 
-function cleanup () {
-  rimraf.sync(PKG_DIR)
+function cleanup (cb) {
+  rimraf(PKG_DIR, cb)
 }
 
 test('setup', function (t) {
@@ -88,8 +88,7 @@ test('allPackageMetadata full request', function (t) {
       }
     }, 'cache contents based on what was written')
     server.done()
-    cleanup()
-    t.end()
+    cleanup(t.end)
   })
 })
 
@@ -126,8 +125,7 @@ test('allPackageMetadata cache only', function (t) {
     t.ok(fileData, 'cache contents written to the right file')
     t.deepEquals(fileData, cacheContents, 'cacheContents written directly')
     server.done()
-    cleanup()
-    t.end()
+    cleanup(t.end)
   })
 })
 
@@ -188,8 +186,7 @@ test('createEntryStream merged stream', function (t) {
     t.ok(fileData, 'cache contents written to the right file')
     t.deepEquals(fileData, cacheContents, 'cache updated correctly')
     server.done()
-    cleanup()
-    t.end()
+    cleanup(t.end)
   })
 })
 
@@ -205,14 +202,11 @@ test('allPackageMetadata no sources', function (t) {
     t.ok(err, 'no sources, got an error')
     t.match(err.message, /No search sources available/, 'useful error message')
     server.done()
-    cleanup()
-    t.end()
+    cleanup(t.end)
   })
 })
 
 test('cleanup', function (t) {
-  cleanup()
   server.close()
-  t.pass('all done')
-  t.done()
+  cleanup(t.end)
 })