Skip to content

Commit

Permalink
fix(tests): move more tests to use real npm
Browse files Browse the repository at this point in the history
This moves a handful of the smaller tests to using the new npm mock that
uses the real actual npm object.  It also extends the testing surface
area of a few tests back down into the actual `process.spawn` that
results, instead of anything internal to the code.

Some dead code in `lib/test.js` was found during this, as well as an
instance of a module throwing a string instead of an error object.

PR-URL: #3463
Credit: @wraithgar
Close: #3463
Reviewed-by: @nlf
  • Loading branch information
wraithgar committed Jul 29, 2021
1 parent e7e1181 commit 6a8086e
Show file tree
Hide file tree
Showing 18 changed files with 272 additions and 204 deletions.
2 changes: 1 addition & 1 deletion lib/set.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class Set extends BaseCommand {

exec (args, cb) {
if (!args.length)
return cb(this.usage)
return cb(this.usageError())
this.npm.commands.config(['set'].concat(args), cb)
}
}
Expand Down
10 changes: 0 additions & 10 deletions lib/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,5 @@ class Test extends LifecycleCmd {
'script-shell',
]
}

exec (args, cb) {
super.exec(args, er => {
if (er && er.code === 'ELIFECYCLE') {
/* eslint-disable standard/no-callback-literal */
cb('Test failed. See above for more details.')
} else
cb(er)
})
}
}
module.exports = Test
1 change: 1 addition & 0 deletions node_modules/.gitignore

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@
"eslint-plugin-promise": "^5.1.0",
"eslint-plugin-standard": "^5.0.0",
"licensee": "^8.2.0",
"spawk": "^1.7.1",
"tap": "^15.0.9"
},
"scripts": {
Expand Down
9 changes: 8 additions & 1 deletion test/fixtures/mock-npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ for (const level in npmlog.levels)
const { title, execPath } = process

const RealMockNpm = (t, otherMocks = {}) => {
t.afterEach(() => {
outputs.length = 0
logs.length = 0
})
t.teardown(() => {
npm.perfStop()
npmlog.record.length = 0
Expand All @@ -22,6 +26,9 @@ const RealMockNpm = (t, otherMocks = {}) => {
})
const logs = []
const outputs = []
const joinedOutput = () => {
return outputs.map(o => o.join(' ')).join('\n')
}
const npm = t.mock('../../lib/npm.js', otherMocks)
const command = async (command, args = []) => {
return new Promise((resolve, reject) => {
Expand All @@ -43,7 +50,7 @@ const RealMockNpm = (t, otherMocks = {}) => {
}
}
npm.output = (...msg) => outputs.push(msg)
return { npm, logs, outputs, command }
return { npm, logs, outputs, command, joinedOutput }
}

const realConfig = require('../../lib/utils/config')
Expand Down
36 changes: 19 additions & 17 deletions test/lib/find-dupes.js
Original file line number Diff line number Diff line change
@@ -1,24 +1,26 @@
const t = require('tap')

const FindDupes = require('../../lib/find-dupes.js')
const { real: mockNpm } = require('../fixtures/mock-npm')

t.test('should run dedupe in dryRun mode', (t) => {
t.plan(3)
const findDupesTest = new FindDupes({
config: {
set: (k, v) => {
t.match(k, 'dry-run')
t.match(v, true)
},
t.test('should run dedupe in dryRun mode', async (t) => {
t.plan(5)
const { npm, command } = mockNpm(t, {
'@npmcli/arborist': function (args) {
t.ok(args, 'gets options object')
t.ok(args.path, 'gets path option')
t.ok(args.dryRun, 'is called in dryRun mode')
this.dedupe = () => {
t.ok(true, 'dedupe is called')
}
},
commands: {
dedupe: (args, cb) => {
t.match(args, [])
cb()
},
'../../lib/utils/reify-finish.js': (npm, arb) => {
t.ok(arb, 'gets arborist tree')
},
})
findDupesTest.exec({}, () => {
t.end()
})
await npm.load()
// explicitly set to false so we can be 100% sure it's always true when it
// hits arborist
npm.config.set('dry-run', false)
npm.config.set('prefix', 'foo')
await command('find-dupes')
})
26 changes: 13 additions & 13 deletions test/lib/get.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
const t = require('tap')
const { real: mockNpm } = require('../fixtures/mock-npm')

t.test('should retrieve values from npm.commands.config', (t) => {
const Get = t.mock('../../lib/get.js')
const get = new Get({
commands: {
config: ([action, arg]) => {
t.equal(action, 'get', 'should use config get action')
t.equal(arg, 'foo', 'should use expected key')
t.end()
},
},
})

get.exec(['foo'])
t.test('should retrieve values from config', async t => {
const { joinedOutput, command, npm } = mockNpm(t)
const name = 'editor'
const value = 'vigor'
await npm.load()
npm.config.set(name, value)
await command('get', [name])
t.equal(
joinedOutput(),
value,
'outputs config item'
)
})
22 changes: 12 additions & 10 deletions test/lib/load-all.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,24 @@
const t = require('tap')
const glob = require('glob')
const { resolve } = require('path')
const { real: mockNpm } = require('../fixtures/mock-npm')

const full = process.env.npm_lifecycle_event === 'check-coverage'

if (!full)
t.pass('nothing to do here, not checking for full coverage')
else {
// some files do config.get() on load, so have to load npm first
const npm = require('../../lib/npm.js')
t.test('load npm first', t => npm.load(t.end))
const { npm } = mockNpm(t)

t.teardown(() => {
const exitHandler = require('../../lib/utils/exit-handler.js')
exitHandler.setNpm(npm)
exitHandler()
})

t.test('load npm first', async t => {
await npm.load()
})

t.test('load all the files', t => {
// just load all the files so we measure coverage for the missing tests
Expand All @@ -21,11 +30,4 @@ else {
t.pass('loaded all files')
t.end()
})

t.test('call the exit handler so we dont freak out', t => {
const exitHandler = require('../../lib/utils/exit-handler.js')
exitHandler.setNpm(npm)
exitHandler()
t.end()
})
}
26 changes: 10 additions & 16 deletions test/lib/prefix.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,13 @@
const t = require('tap')
const { real: mockNpm } = require('../fixtures/mock-npm')

t.test('prefix', (t) => {
t.plan(3)
const dir = '/prefix/dir'

const Prefix = require('../../lib/prefix.js')
const prefix = new Prefix({
prefix: dir,
output: (output) => {
t.equal(output, dir, 'prints the correct directory')
},
})

prefix.exec([], (err) => {
t.error(err, 'npm prefix')
t.ok('should have printed directory')
})
t.test('prefix', async (t) => {
const { joinedOutput, command, npm } = mockNpm(t)
await npm.load()
await command('prefix')
t.equal(
joinedOutput(),
npm.prefix,
'outputs npm.prefix'
)
})
20 changes: 6 additions & 14 deletions test/lib/prune.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
const t = require('tap')
const { real: mockNpm } = require('../fixtures/mock-npm')

t.test('should prune using Arborist', (t) => {
const Prune = t.mock('../../lib/prune.js', {
t.test('should prune using Arborist', async (t) => {
t.plan(4)
const { command, npm } = mockNpm(t, {
'@npmcli/arborist': function (args) {
t.ok(args, 'gets options object')
t.ok(args.path, 'gets path option')
Expand All @@ -13,16 +15,6 @@ t.test('should prune using Arborist', (t) => {
t.ok(arb, 'gets arborist tree')
},
})
const prune = new Prune({
prefix: 'foo',
flatOptions: {
foo: 'bar',
},
})
prune.exec(null, er => {
if (er)
throw er
t.ok(true, 'callback is called')
t.end()
})
await npm.load()
await command('prune')
})
48 changes: 34 additions & 14 deletions test/lib/restart.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,36 @@
const t = require('tap')
let runArgs
const npm = {
commands: {
'run-script': (args, cb) => {
runArgs = args
cb()
},
},
}
const Restart = require('../../lib/restart.js')
const restart = new Restart(npm)
restart.exec(['foo'], () => {
t.match(runArgs, ['restart', 'foo'])
t.end()
const spawk = require('spawk')
const { real: mockNpm } = require('../fixtures/mock-npm')

spawk.preventUnmatched()
t.teardown(() => {
spawk.unload()
})

// TODO this ... smells. npm "script-shell" config mentions defaults but those
// are handled by run-script, not npm. So for now we have to tie tests to some
// pretty specific internals of runScript
const makeSpawnArgs = require('@npmcli/run-script/lib/make-spawn-args.js')

t.test('should run stop script from package.json', async t => {
const prefix = t.testdir({
'package.json': JSON.stringify({
name: 'x',
version: '1.2.3',
scripts: {
restart: 'node ./test-restart.js',
},
}),
})
const { command, npm } = mockNpm(t)
await npm.load()
npm.log.level = 'silent'
npm.localPrefix = prefix
const [scriptShell] = makeSpawnArgs({ path: prefix })
const script = spawk.spawn(scriptShell, (args) => {
t.ok(args.includes('node ./test-restart.js "foo"'), 'ran stop script with extra args')
return true
})
await command('restart', ['foo'])
t.ok(script.called, 'script ran')
})
26 changes: 10 additions & 16 deletions test/lib/root.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,13 @@
const t = require('tap')
const { real: mockNpm } = require('../fixtures/mock-npm')

t.test('root', (t) => {
t.plan(3)
const dir = '/root/dir'

const Root = require('../../lib/root.js')
const root = new Root({
dir,
output: (output) => {
t.equal(output, dir, 'prints the correct directory')
},
})

root.exec([], (err) => {
t.error(err, 'npm root')
t.ok('should have printed directory')
})
t.test('prefix', async (t) => {
const { joinedOutput, command, npm } = mockNpm(t)
await npm.load()
await command('root')
t.equal(
joinedOutput(),
npm.dir,
'outputs npm.dir'
)
})
27 changes: 27 additions & 0 deletions test/lib/set.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,32 @@
const t = require('tap')

// can't run this until npm set can save to npm.localPrefix
t.skip('npm set', async t => {
const { real: mockNpm } = require('../fixtures/mock-npm')
const { joinedOutput, command, npm } = mockNpm(t)
await npm.load()

t.test('no args', async t => {
t.rejects(
command('set'),
/Usage:/,
'prints usage'
)
})

t.test('test-config-item', async t => {
npm.localPrefix = t.testdir({})
t.not(npm.config.get('test-config-item', 'project'), 'test config value', 'config is not already new value')
// This will write to ~/.npmrc!
// Don't unskip until we can write to project level
await command('set', ['test-config-item=test config value'])
t.equal(joinedOutput(), '', 'outputs nothing')
t.equal(npm.config.get('test-config-item', 'project'), 'test config value', 'config is set to new value')
})
})

// Everything after this can go away once the above test is unskipped

let configArgs = null
const npm = {
commands: {
Expand Down
Loading

0 comments on commit 6a8086e

Please sign in to comment.