-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
assert: adds rejects() and doesNotReject() #18023
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -425,14 +425,23 @@ function getActual(block) { | |
return NO_EXCEPTION_SENTINEL; | ||
} | ||
|
||
// Expected to throw an error. | ||
assert.throws = function throws(block, error, message) { | ||
const actual = getActual(block); | ||
async function waitForActual(block) { | ||
if (typeof block !== 'function') { | ||
throw new ERR_INVALID_ARG_TYPE('block', 'Function', block); | ||
} | ||
try { | ||
await block(); | ||
} catch (e) { | ||
return e; | ||
} | ||
return NO_EXCEPTION_SENTINEL; | ||
} | ||
|
||
function expectsError(stackStartFn, actual, error, message) { | ||
if (typeof error === 'string') { | ||
if (arguments.length === 3) | ||
if (arguments.length === 4) { | ||
throw new ERR_INVALID_ARG_TYPE('error', ['Function', 'RegExp'], error); | ||
|
||
} | ||
message = error; | ||
error = null; | ||
} | ||
|
@@ -443,21 +452,21 @@ assert.throws = function throws(block, error, message) { | |
details += ` (${error.name})`; | ||
} | ||
details += message ? `: ${message}` : '.'; | ||
const fnType = stackStartFn === rejects ? 'rejection' : 'exception'; | ||
innerFail({ | ||
actual, | ||
expected: error, | ||
operator: 'throws', | ||
message: `Missing expected exception${details}`, | ||
stackStartFn: throws | ||
operator: stackStartFn.name, | ||
message: `Missing expected ${fnType}${details}`, | ||
stackStartFn | ||
}); | ||
} | ||
if (error && expectedException(actual, error, message) === false) { | ||
throw actual; | ||
} | ||
}; | ||
} | ||
|
||
assert.doesNotThrow = function doesNotThrow(block, error, message) { | ||
const actual = getActual(block); | ||
function expectsNoError(stackStartFn, actual, error, message) { | ||
if (actual === NO_EXCEPTION_SENTINEL) | ||
return; | ||
|
||
|
@@ -468,16 +477,41 @@ assert.doesNotThrow = function doesNotThrow(block, error, message) { | |
|
||
if (!error || expectedException(actual, error)) { | ||
const details = message ? `: ${message}` : '.'; | ||
const fnType = stackStartFn === doesNotReject ? 'rejection' : 'exception'; | ||
innerFail({ | ||
actual, | ||
expected: error, | ||
operator: 'doesNotThrow', | ||
message: `Got unwanted exception${details}\n${actual && actual.message}`, | ||
stackStartFn: doesNotThrow | ||
operator: stackStartFn.name, | ||
message: `Got unwanted ${fnType}${details}\n${actual && actual.message}`, | ||
stackStartFn | ||
}); | ||
} | ||
throw actual; | ||
}; | ||
} | ||
|
||
function throws(block, ...args) { | ||
expectsError(throws, getActual(block), ...args); | ||
} | ||
|
||
assert.throws = throws; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (non-blocking): I personally prefer the following to save lines: assert.throws = function throws(block, ...args) {
expectsError(throws, getActual(block), ...args);
}; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. We could check for the function name instead but that could theoretically be manipulated. So just keep it as is. Thanks for the heads up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, while looking at the code again: we actually already use the function name and do not strictly test if it it a "valid" function. So we could indeed just switch to name checking only. |
||
|
||
async function rejects(block, ...args) { | ||
expectsError(rejects, await waitForActual(block), ...args); | ||
} | ||
|
||
assert.rejects = rejects; | ||
|
||
function doesNotThrow(block, ...args) { | ||
expectsNoError(doesNotThrow, getActual(block), ...args); | ||
} | ||
|
||
assert.doesNotThrow = doesNotThrow; | ||
|
||
async function doesNotReject(block, ...args) { | ||
expectsNoError(doesNotReject, await waitForActual(block), ...args); | ||
} | ||
|
||
assert.doesNotReject = doesNotReject; | ||
|
||
assert.ifError = function ifError(err) { | ||
if (err !== null && err !== undefined) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const { promisify } = require('util'); | ||
const wait = promisify(setTimeout); | ||
|
||
/* eslint-disable prefer-common-expectserror, no-restricted-properties */ | ||
|
||
// Test assert.rejects() and assert.doesNotReject() by checking their | ||
// expected output and by verifying that they do not work sync | ||
|
||
assert.rejects( | ||
() => assert.fail(), | ||
common.expectsError({ | ||
code: 'ERR_ASSERTION', | ||
type: assert.AssertionError, | ||
message: 'Failed' | ||
}) | ||
); | ||
|
||
assert.doesNotReject(() => {}); | ||
|
||
{ | ||
const promise = assert.rejects(async () => { | ||
await wait(1); | ||
assert.fail(); | ||
}, common.expectsError({ | ||
code: 'ERR_ASSERTION', | ||
type: assert.AssertionError, | ||
message: 'Failed' | ||
})); | ||
assert.doesNotReject(() => promise); | ||
} | ||
|
||
{ | ||
const promise = assert.doesNotReject(async () => { | ||
await wait(1); | ||
throw new Error(); | ||
}); | ||
assert.rejects(() => promise, | ||
(err) => { | ||
assert(err instanceof assert.AssertionError, | ||
`${err.name} is not instance of AssertionError`); | ||
assert.strictEqual(err.code, 'ERR_ASSERTION'); | ||
assert(/^Got unwanted rejection\.\n$/.test(err.message)); | ||
assert.strictEqual(err.operator, 'doesNotReject'); | ||
assert.ok(!err.stack.includes('at Function.doesNotReject')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without stack trace cleaning:
With cleaning:
|
||
return true; | ||
} | ||
); | ||
} | ||
|
||
{ | ||
const promise = assert.rejects(() => {}); | ||
assert.rejects(() => promise, | ||
(err) => { | ||
assert(err instanceof assert.AssertionError, | ||
`${err.name} is not instance of AssertionError`); | ||
assert.strictEqual(err.code, 'ERR_ASSERTION'); | ||
assert(/^Missing expected rejection\.$/.test(err.message)); | ||
assert.strictEqual(err.operator, 'rejects'); | ||
assert.ok(!err.stack.includes('at Function.rejects')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without stack trace cleaning:
With cleaning:
|
||
return true; | ||
} | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,6 +116,7 @@ assert.throws(() => thrower(TypeError)); | |
} catch (e) { | ||
threw = true; | ||
assert.ok(e instanceof a.AssertionError); | ||
assert.ok(!e.stack.includes('at Function.doesNotThrow')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As requested, I removed the unnecessary test, and enriched an existing one. Without stack trace cleaning:
With cleaning:
|
||
} | ||
assert.ok(threw, 'a.doesNotThrow is not catching type matching errors'); | ||
} | ||
|
@@ -221,6 +222,16 @@ a.throws(() => thrower(TypeError), (err) => { | |
code: 'ERR_ASSERTION', | ||
message: /^Missing expected exception \(TypeError\): fhqwhgads$/ | ||
})); | ||
|
||
let threw = false; | ||
try { | ||
a.throws(noop); | ||
} catch (e) { | ||
threw = true; | ||
assert.ok(e instanceof a.AssertionError); | ||
assert.ok(!e.stack.includes('at Function.throws')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As requested, I reverted the existing test.
Without stack trace cleaning:
With cleaning:
|
||
} | ||
assert.ok(threw); | ||
} | ||
|
||
const circular = { y: 1 }; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This async wrapper is really ugly.
But If I omit it, the linter cannot even parse this code (top-level await is not for tomorrow...).
The other option would be to remove
await
, but it doesn't highlight the async nature ofdoesNotReject()
.What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to keep it as is.