Skip to content

Commit

Permalink
test: Clean up some flakiness and inconsistency
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
isaacs authored and ruyadorno committed Oct 31, 2019
1 parent 3e7ed30 commit 9a2d8af
Show file tree
Hide file tree
Showing 109 changed files with 1,114 additions and 2,814 deletions.
6 changes: 2 additions & 4 deletions test/common-tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
18 changes: 6 additions & 12 deletions test/tap/404-parent.js
Original file line number Diff line number Diff line change
@@ -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')
Expand All @@ -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',
Expand Down
10 changes: 3 additions & 7 deletions test/tap/access.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
})

Expand All @@ -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)
})
})

Expand Down Expand Up @@ -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)
}
)
})
Expand Down Expand Up @@ -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()
Expand Down
24 changes: 3 additions & 21 deletions test/tap/add-remote-git-file.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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')

Expand Down Expand Up @@ -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)
Expand All @@ -95,9 +83,3 @@ function setup (cb) {
}, cb)
})
}

function cleanup () {
process.chdir(osenv.tmpdir())
rimraf.sync(repo)
rimraf.sync(pkg)
}
26 changes: 5 additions & 21 deletions test/tap/add-remote-git-shrinkwrap.js
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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')

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -145,9 +135,3 @@ function setup (cb) {
}, cb)
})
}

function cleanup () {
process.chdir(osenv.tmpdir())
rimraf.sync(repo)
rimraf.sync(pkg)
}
15 changes: 4 additions & 11 deletions test/tap/add-remote-git-submodule.js
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -141,9 +140,3 @@ function setup (cb) {
}, cb)
})
}

function cleanup () {
process.chdir(osenv.tmpdir())
rimraf.sync(repos)
rimraf.sync(pkg)
}
27 changes: 5 additions & 22 deletions test/tap/add-remote-git.js
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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')

Expand All @@ -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)
Expand Down Expand Up @@ -113,9 +102,3 @@ function setup (cb) {
}, cb)
})
}

function cleanup () {
process.chdir(osenv.tmpdir())
rimraf.sync(repo)
rimraf.sync(pkg)
}
20 changes: 7 additions & 13 deletions test/tap/all-package-metadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
})
})

Expand Down Expand Up @@ -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)
})
})

Expand Down Expand Up @@ -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)
})
})

Expand All @@ -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)
})
Loading

0 comments on commit 9a2d8af

Please sign in to comment.