Skip to content

Commit

Permalink
fix: store unref promises for awaiting in tests
Browse files Browse the repository at this point in the history
  • Loading branch information
lukekarrys committed Apr 24, 2024
1 parent e5f1948 commit 06202f0
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 10 deletions.
10 changes: 8 additions & 2 deletions lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class Npm {
return require(`./commands/${command}.js`)
}

unrefPromises = []
updateNotification = null
argv = []

Expand Down Expand Up @@ -224,11 +225,16 @@ class Npm {
log.verbose('argv', this.#argvClean.map(JSON.stringify).join(' '))
})

this.#logFile.load({
// logFile.load returns a promise that resolves when old logs are done being cleaned.
// We save this promise to an array so that we can await it in tests to ensure more
// deterministic logging behavior. The process will also hang open if this were to
// take a long time to resolve, but that is why process.exit is called explicitly
// in the exit-handler.
this.unrefPromises.push(this.#logFile.load({
path: this.logPath,
logsMax: this.config.get('logs-max'),
timing: this.config.get('timing'),
})
}))

this.#timers.load({
path: this.logPath,
Expand Down
11 changes: 11 additions & 0 deletions test/fixtures/mock-npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,17 @@ const getMockNpm = async (t, { mocks, init, load, npm: npmOpts }) => {
})
}

async load () {
const res = await super.load()
// Wait for any promises (currently only log file cleaning) to be
// done before returning from load in tests. This helps create more
// deterministic testing behavior because in reality that promise
// is left hanging on purpose as a best-effort and the process gets
// closed regardless of if it has finished or not.
await Promise.all(this.unrefPromises)
return res
}

async exec (...args) {
const [res, err] = await super.exec(...args).then((r) => [r]).catch(e => [null, e])
// This mimics how the exit handler flushes output for commands that have
Expand Down
9 changes: 1 addition & 8 deletions test/lib/cli/exit-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,6 @@ t.test('handles unknown error with logs and debug file', async (t) => {
})

await exitHandler(err('Unknown error', 'ECODE'))
// force logfile cleaning logs to happen since those are purposefully not awaited
await require('timers/promises').setTimeout(200)

const fileLogs = await debugFile()
const fileLines = fileLogs.split('\n')
Expand All @@ -140,19 +138,14 @@ t.test('handles unknown error with logs and debug file', async (t) => {

t.equal(process.exitCode, 1)

let skippedLogs = 0
logs.forEach((logItem, i) => {
const logLines = logItem.split('\n').map(l => `${i} ${l}`)
for (const line of logLines) {
if (line.includes('logfile') && line.includes('cleaning')) {
skippedLogs++
continue
}
t.match(fileLogs.trim(), line, 'log appears in debug file')
}
})

t.equal(logs.length - skippedLogs, parseInt(lastLog) + 1)
t.equal(logs.length, parseInt(lastLog) + 1)
t.match(logs.error, [
'code ECODE',
'ERR SUMMARY Unknown error',
Expand Down

0 comments on commit 06202f0

Please sign in to comment.