-
-
Notifications
You must be signed in to change notification settings - Fork 34.9k
test_runner: support custom message for expectFailure #61563
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
base: main
Are you sure you want to change the base?
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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||
| 'use strict'; | ||||||||||
| const { | ||||||||||
| ArrayPrototypeEvery, | ||||||||||
| ArrayPrototypePush, | ||||||||||
| ArrayPrototypePushApply, | ||||||||||
| ArrayPrototypeShift, | ||||||||||
|
|
@@ -13,6 +14,7 @@ const { | |||||||||
| MathMax, | ||||||||||
| Number, | ||||||||||
| NumberPrototypeToFixed, | ||||||||||
| ObjectKeys, | ||||||||||
| ObjectSeal, | ||||||||||
| Promise, | ||||||||||
| PromisePrototypeThen, | ||||||||||
|
|
@@ -40,6 +42,7 @@ const { | |||||||||
| AbortError, | ||||||||||
| codes: { | ||||||||||
| ERR_INVALID_ARG_TYPE, | ||||||||||
| ERR_INVALID_ARG_VALUE, | ||||||||||
| ERR_TEST_FAILURE, | ||||||||||
| }, | ||||||||||
| } = require('internal/errors'); | ||||||||||
|
|
@@ -56,7 +59,8 @@ const { | |||||||||
| once: runOnce, | ||||||||||
| setOwnProperty, | ||||||||||
| } = require('internal/util'); | ||||||||||
| const { isPromise } = require('internal/util/types'); | ||||||||||
| const assert = require('assert'); | ||||||||||
| const { isPromise, isRegExp } = require('internal/util/types'); | ||||||||||
| const { | ||||||||||
| validateAbortSignal, | ||||||||||
| validateFunction, | ||||||||||
|
|
@@ -487,6 +491,39 @@ class SuiteContext { | |||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| function parseExpectFailure(expectFailure) { | ||||||||||
| if (expectFailure === undefined || expectFailure === false) { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (typeof expectFailure === 'string') { | ||||||||||
| return { __proto__: null, label: expectFailure, match: undefined }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (typeof expectFailure === 'function' || isRegExp(expectFailure)) { | ||||||||||
| return { __proto__: null, label: undefined, match: expectFailure }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (typeof expectFailure !== 'object') { | ||||||||||
| return { __proto__: null, label: undefined, match: undefined }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const keys = ObjectKeys(expectFailure); | ||||||||||
| if (keys.length === 0) { | ||||||||||
| throw new ERR_INVALID_ARG_VALUE('options.expectFailure', expectFailure, 'must not be an empty object'); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (ArrayPrototypeEvery(keys, (k) => k === 'match' || k === 'label')) { | ||||||||||
| return { | ||||||||||
| __proto__: null, | ||||||||||
| label: expectFailure.label, | ||||||||||
| match: expectFailure.match, | ||||||||||
| }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return { __proto__: null, label: undefined, match: expectFailure }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| class Test extends AsyncResource { | ||||||||||
| reportedType = 'test'; | ||||||||||
| abortController; | ||||||||||
|
|
@@ -636,7 +673,7 @@ class Test extends AsyncResource { | |||||||||
| this.plan = null; | ||||||||||
| this.expectedAssertions = plan; | ||||||||||
| this.cancelled = false; | ||||||||||
| this.expectFailure = expectFailure !== undefined && expectFailure !== false; | ||||||||||
| this.expectFailure = parseExpectFailure(expectFailure) || this.parent?.expectFailure; | ||||||||||
| this.skipped = skip !== undefined && skip !== false; | ||||||||||
| this.isTodo = (todo !== undefined && todo !== false) || this.parent?.isTodo; | ||||||||||
| this.startTime = null; | ||||||||||
|
|
@@ -950,7 +987,30 @@ class Test extends AsyncResource { | |||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (this.expectFailure === true) { | ||||||||||
| if (this.expectFailure) { | ||||||||||
| if (typeof this.expectFailure === 'object' && | ||||||||||
| this.expectFailure.match !== undefined) { | ||||||||||
| const { match: validation } = this.expectFailure; | ||||||||||
| try { | ||||||||||
| const errorToCheck = ( | ||||||||||
| err?.code === 'ERR_TEST_FAILURE' && | ||||||||||
| err?.failureType === kTestCodeFailure && | ||||||||||
| err.cause | ||||||||||
| ) ? | ||||||||||
| err.cause : | ||||||||||
| err; | ||||||||||
| // eslint-disable-next-line no-restricted-syntax | ||||||||||
| assert.throws(() => { throw errorToCheck; }, validation); | ||||||||||
| } catch (e) { | ||||||||||
| this.passed = false; | ||||||||||
| this.error = new ERR_TEST_FAILURE( | ||||||||||
| 'The test failed, but the error did not match the expected validation', | ||||||||||
| kTestCodeFailure, | ||||||||||
| ); | ||||||||||
| this.error.cause = e; | ||||||||||
| return; | ||||||||||
| } | ||||||||||
| } | ||||||||||
| this.passed = true; | ||||||||||
| } else { | ||||||||||
| this.passed = false; | ||||||||||
|
|
@@ -960,7 +1020,7 @@ class Test extends AsyncResource { | |||||||||
| } | ||||||||||
|
|
||||||||||
| pass() { | ||||||||||
| if (this.error == null && this.expectFailure === true && !this.skipped) { | ||||||||||
| if (this.error == null && this.expectFailure && !this.skipped) { | ||||||||||
| this.passed = false; | ||||||||||
| this.error = new ERR_TEST_FAILURE( | ||||||||||
| 'test was expected to fail but passed', | ||||||||||
|
|
@@ -972,6 +1032,20 @@ class Test extends AsyncResource { | |||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (this.skipped || this.isTodo) { | ||||||||||
| this.passed = true; | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (this.expectFailure) { | ||||||||||
| this.passed = false; | ||||||||||
| this.error = new ERR_TEST_FAILURE( | ||||||||||
| 'Test passed but was expected to fail', | ||||||||||
| kTestCodeFailure, | ||||||||||
| ); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| this.passed = true; | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -1361,7 +1435,10 @@ class Test extends AsyncResource { | |||||||||
| } else if (this.isTodo) { | ||||||||||
| directive = this.reporter.getTodo(this.message); | ||||||||||
| } else if (this.expectFailure) { | ||||||||||
| directive = this.reporter.getXFail(this.expectFailure); // TODO(@JakobJingleheimer): support specifying failure | ||||||||||
| const message = typeof this.expectFailure === 'object' ? | ||||||||||
| this.expectFailure.label : | ||||||||||
| this.expectFailure; | ||||||||||
|
Comment on lines
+1438
to
+1440
Member
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:
Suggested change
Contributor
Author
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. If we use || this.expectFailure, the message variable incorrectly becomes an object (e.g., { label: undefined, ... }) instead of a string or undefined. This happens because this.expectFailure is internally normalized to an object even for boolean inputs. The test runner expects a string message, and passing an object causes serialization errors.
Member
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. Ohhh 👍 That's weird 🤔
Contributor
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. These sorts of implementation subtleties are prone to unintended behavioral changes as the code evolves. I suggest having more corner case coverage to lock down ambiguities and facilitate further review, e.g. #61563 (review)
Member
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. Oh, actually, that's a good point. I think we can avoid any logic here by making either type ExpectFailure = false | {
label: string, // empty string when user did not provide
matcher: Matcher | nullish,
}or type ExpectFailure = false | {
label?: string,
match?: Matcher,
}Regardless of how the user provided Ex test.expectFailure('whatever', () => {…});this.expectFailure = {};test('whatever', { expectFailure: '#42' }, () => {…});this.expectFailure = { label: '#42' };test('whatever', { expectFailure: { label: '#42' } }, () => {…});this.expectFailure = { label: '#42' }; |
||||||||||
| directive = this.reporter.getXFail(message); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (this.reportedType) { | ||||||||||
|
|
||||||||||
Uh oh!
There was an error while loading. Please reload this page.