-
-
Notifications
You must be signed in to change notification settings - Fork 34.6k
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?
Conversation
Update `expectFailure` option to accept a string message and display it in the TAP reporter output. Example output: `# EXPECTED FAILURE <reason>`
|
Review requested:
|
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.
We already have a plan for the value of expectFail: it will soon accept a regular expression to match against.
@JakobJingleheimer In that case, I'd be happy to pivot this PR to implement the expectFailure validation logic (accepting a string/regex to match the error) instead of just a message. Does that sound good, or is there someone else already working on it?" |
|
@vassudanagunta you were part of the original discussion; did you happen to start an implementation? To my knowledge though, no-one has started. I had planned to pick it up next week, but if you would like to do, go ahead. If you do, I think it would probably be better to start a new PR than to pivot this one. So open a draft and I'll add it to the test-runner team's kanban board so it gets proper visibility. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61563 +/- ##
=======================================
Coverage 89.76% 89.76%
=======================================
Files 673 673
Lines 203788 203880 +92
Branches 39168 39193 +25
=======================================
+ Hits 182926 183020 +94
- Misses 13189 13190 +1
+ Partials 7673 7670 -3
🚀 New features to boost your workflow:
|
|
@JakobJingleheimer nope, haven't started this, though I had long ago implemented it in I think it's important to get the requirements nailed. IMHO, #61570. |
|
As I said, let's put together a proposal in the nodejs/test_runner repo 🙂 |
? I should start a discussion in that repo? |
|
reviewed before reading the discussion; imo a string should work as in this PR whether or not it also supports accepting a regex. |
|
It could do. My concern is supporting this without considering the intended regex feature accidentally precluding that intended feature, or inadvertently creating a breaking change, or creating heavily conflicting PRs (very frustrating for the implementators). I think we can likely get both; we can easily avoid those problems with a quick proposal so everyone is on the same page 🙂
Please start a proposal like the ones already in that repo 🙂 https://github.com/nodejs/test-runner/tree/main/proposals we can discuss it in that PR |
|
conflicts fair; as long as the "should expect failure" uses truthiness (does an empty string count as true or false, though?), i can't foresee any semantic collision. |
1af3584 to
13aedaa
Compare
|
I've opened a proposal PR in the test-runner repository as suggested by @JakobJingleheimer. |
Update `expectFailure` to accept an object for detailed configuration. - Support `message` property for TAP output directives. - Support `with` property for error validation (RegExp or Object), similar to `assert.throws`. Tests added in `test/parallel/test-runner-xfail.js`.
Enhance `expectFailure` option to accep - Add `message` property for custom TAP directives. - Add `with` property for error validation using `assert.throws`. Tests added in `test/parallel/test-runner-xfail.js`.
Bypass `no-restricted-syntax` ("Only use simple assertions") in failure validation logic by aliasing `assert.throws`.
| expectFailure: { | ||
| with: /error message/, | ||
| message: 'reason for failure', | ||
| }, |
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 like these field names the most compared to all the other ideas 👍🏾. Put it in the proposal?
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 only just learned that pressing the comment button is not enough. you have to then submit the comment. this is a really bad GitHub UX)
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.
The following are equivalent, yes?
expectFailure: 'reason for failure'
expectFailure: {message: 'reason for failure'}
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.
Updated the proposal based on the feedback:
- Added support for Direct Matchers (e.g.,
expectFailure: /error/). - Clarified that
withis for validation andmessageis for reasoning. - Noted that
expectFailure: 'reason'and{ message: 'reason' }are equivalent.
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.
Added support for Direct Matchers (e.g., expectFailure: /error/).
This means that the following are also equivalent, yes?
expectFailure: /error/
expectFailure: {with: /error/}
and likewise any other value type accepted by assert.throws, yes?
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.
Similarly, the following are also equivalent, yes?
expectFailure: true
expectFailure: {}
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.
Yes, exactly.
Summary
Update
expectFailureoption to accept a string message and display it in the TAP reporter output.Example output:
# EXPECTED FAILURE <reason>This aligns
expectFailurebehavior withskipandtodowhich already support custom messages.Test Plan
Added a new test file
test/parallel/test-runner-xfail-message.jsthat verifies:expectFailureaccepts a string.Documentation
Updated
doc/api/test.mdto include:expectFailurein theoptionslist fortest().expectFailurewith a message.