Skip to content

Commit

Permalink
Properly handle test aborts when awaiting promises
Browse files Browse the repository at this point in the history
Fix #583

Add an 'abort' method to the Waiter class in lib/test.js.  This lets us
effectively bail out of a promise when `t.endAll()` is called.

Previously, using bailouts in the presence of t.resolves, t.rejects,
t.resolveMatch, or t.resolveMatchSnapshot would throw a TypeError,
because the endAll method expected this.occupied to be a Test object.

However, as of 63f9c92, `this.occupied`
is set to a Waiter object instead, to avoid an extra layer of
indentation for promise-handling assertions.
  • Loading branch information
isaacs committed Sep 10, 2019
1 parent 28cd2ff commit 36d6dc5
Show file tree
Hide file tree
Showing 3 changed files with 206 additions and 13 deletions.
41 changes: 28 additions & 13 deletions lib/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,21 +78,36 @@ class Waiter {
this.rejected = false
this.done = false
this.finishing = false
this.expectReject = false
this.promise = new Promise(res => this.resolve = res)
promise.then(value => {
this.resolved = true
this.value = value
this.done = true
this.finish()
}).catch(er => {
this.value = er
this.rejected = true
}).catch(er => this.reject(er))
}
reject (er) {
this.value = er
this.rejected = true
this.done = true
this.finish()
}
abort (er) {
/* istanbul ignore else - an excess of caution, should be impossible */
if (!this.done) {
this.ready = true
this.finishing = false
this.done = true
this.finish()
})
this.value = er
// make it clear that this is a problem by doing
// the opposite of what was requested.
this.rejected = !this.expectReject
return this.finish()
}
}
finish () {
if (this.ready && this.done) {
if (this.ready && this.done && !this.finishing) {
this.finishing = true
this.cb(this)
this.resolve()
Expand Down Expand Up @@ -582,8 +597,6 @@ class Test extends Base {
else
p.setTimeout(false)
this.debug('onindentedend %s(%s)', this.name, p.name)
if (!p.bailedOut && !this.bailedOut)
assert.equal(this.occupied, p)
this.occupied = null
this.debug('OIE(%s) b>shift into queue', this.name, this.queue)
p.options.stack = ''
Expand Down Expand Up @@ -948,11 +961,12 @@ class Test extends Base {
this.processing = true
if (this.occupied) {
const p = this.occupied
if (p.endAll)
if (p instanceof Waiter)
p.abort(new Error('test unfinished'))
else if (p.endAll)
p.endAll(true)
else {
else
p.parser.abort('test unfinished')
}
} else if (sub) {
this.process()
if (queueEmpty(this)) {
Expand Down Expand Up @@ -1470,13 +1484,14 @@ class Test extends Base {
}
}

waitOn (promise, cb) {
waitOn (promise, cb, expectReject) {

This comment has been minimized.

Copy link
@isaacs

isaacs Sep 10, 2019

Author Member

I don't love this "trailing boolean" API surface, but at least it's an internal method. Probably waitOn should be a Symbol to keep it private?

const w = new Waiter(promise, w => {
assert.equal(this.occupied, w)
cb(w)
this.occupied = null
this.process()
})
w.expectReject = !!expectReject
this.queue.push(w)
this.process()
return w.promise
Expand Down Expand Up @@ -1570,7 +1585,7 @@ class Test extends Base {
const actual = isRegExp(wanted) && er ? er.message : er
return wanted ? this.match(actual, wanted, message, extra)
: this.pass(message, extra)
})
}, true)
}

resolves (promise, message, extra) {
Expand Down
125 changes: 125 additions & 0 deletions tap-snapshots/test-test.js-TAP.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,45 @@ not ok 1 - this is the test that never ends # {time}
`

exports[`test/test.js TAP assertions and weird stuff endAll with unresolved t.resolveMatch > output 1`] = `
TAP version 13
# Subtest: this is the test that never ends
# Subtest: it goes on and on my friend
ok 1 - this is ok
not ok 2 - expect resolving Promise
---
at:
line: #
column: #
file: test/test.js
found:
!error
name: Error
message: test unfinished
stack: |
{STACK}
source: |2
tt.pass('this is ok')
tt.resolveMatch(() => new Promise(()=>{}), {})
--^
})
tt.pass('some queue stuff')
...
1..2
# failed 1 of 2 tests
not ok 1 - it goes on and on my friend # {time}
ok 2 - some queue stuff
1..2
# failed 1 of 2 tests
not ok 1 - this is the test that never ends # {time}
1..1
# failed 1 test
`

exports[`test/test.js TAP assertions and weird stuff equal > output 1`] = `
TAP version 13
not ok 1 - should be equal
Expand Down Expand Up @@ -1731,6 +1770,92 @@ not ok 1 - expect fail
`

exports[`test/test.js TAP endAll direct while waiting on Promise rejection > result 1`] = `
TAP version 13
not ok 1 - expect rejected Promise
---
at:
line: #
column: #
file: test/test.js
found:
!error
name: Error
message: test unfinished
stack: |
{STACK}
source: |2
})
tt.rejects(() => new Promise(() => {}), { message: 'never resolves' })
--^
setTimeout(() => tt.endAll())
})
...
1..1
# failed 1 test
`

exports[`test/test.js TAP endAll direct while waiting on a resolving promise > result 1`] = `
TAP version 13
not ok 1 - expect resolving Promise
---
at:
line: #
column: #
file: test/test.js
found:
!error
name: Error
message: test unfinished
stack: |
{STACK}
source: |2
})
tt.resolveMatch(() => new Promise(() => {}), 'never resolves')
--^
setTimeout(() => tt.endAll())
})
...
1..1
# failed 1 test
`

exports[`test/test.js TAP endAll with sub while waiting on a resolving promise > result 1`] = `
TAP version 13
# Subtest
not ok 1 - expect resolving Promise
---
at:
line: #
column: #
file: test/test.js
found:
!error
name: Error
message: test unfinished
stack: |
{STACK}
source: |2
})
tt.test(t => t.resolveMatch(() => new Promise(() => {}), 'never resolves'))
--^
setTimeout(() => tt.endAll())
})
...
1..1
# failed 1 test
not ok 1 # {time}
1..1
# failed 1 test
`

exports[`test/test.js TAP short output checks bailout after end bailout > bailout after end 1`] = `
TAP version 13
# Subtest
Expand Down
53 changes: 53 additions & 0 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,17 @@ t.test('assertions and weird stuff', t => {
tt.endAll()
},

'endAll with unresolved t.resolveMatch': tt => {
tt.test('this is the test that never ends', tt => {
tt.test('it goes on and on my friend', tt => {
tt.pass('this is ok')
tt.resolveMatch(() => new Promise(()=>{}), {})
})
tt.pass('some queue stuff')
})
tt.endAll()
},

'endAll with stdin': tt => {
const s = new MiniPass()
tt.stdin({ tapStream: s })
Expand Down Expand Up @@ -1100,3 +1111,45 @@ t.test('snapshots', async t => {

t.end()
})

t.test('endAll direct while waiting on a resolving promise', t => {
t.plan(1)
const tt = new Test()
tt.setEncoding('utf8')
const buf = []
tt.on('data', c => buf.push(c))
tt.on('end', () => {
const result = buf.join('')
t.matchSnapshot(result, 'result')
})
tt.resolveMatch(() => new Promise(() => {}), 'never resolves')
setTimeout(() => tt.endAll())
})

t.test('endAll direct while waiting on Promise rejection', t => {
t.plan(1)
const tt = new Test()
tt.setEncoding('utf8')
const buf = []
tt.on('data', c => buf.push(c))
tt.on('end', () => {
const result = buf.join('')
t.matchSnapshot(result, 'result')
})
tt.rejects(() => new Promise(() => {}), { message: 'never resolves' })
setTimeout(() => tt.endAll())
})

t.test('endAll with sub while waiting on a resolving promise', t => {
t.plan(1)
const tt = new Test()
tt.setEncoding('utf8')
const buf = []
tt.on('data', c => buf.push(c))
tt.on('end', () => {
const result = buf.join('')
t.matchSnapshot(result, 'result')
})
tt.test(t => t.resolveMatch(() => new Promise(() => {}), 'never resolves'))
setTimeout(() => tt.endAll())
})

0 comments on commit 36d6dc5

Please sign in to comment.