-
-
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
|
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61563 +/- ##
==========================================
- Coverage 89.74% 89.73% -0.02%
==========================================
Files 674 674
Lines 204360 204433 +73
Branches 39270 39301 +31
==========================================
+ Hits 183409 183443 +34
- Misses 13260 13308 +48
+ Partials 7691 7682 -9
🚀 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. |
| 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.
@JakobJingleheimer and the test runner team as a whole: the discussion of this feature is now occurring in two places... far from ideal and likely to happen again and again because the natural place to have such discussions is on the Issues and PRs made in the nodejs repo.
I was gonna say corralling all discussion to a separate repo will be like corralling cats... but maybe the unconventional split of test runner code and test runner discussion is the cat?
I think it's worth the test runner team addressing this.
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.
It should all happen in the proposal (I believe I suggested closing this PR in the interim; it can always be re-opened).
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've included it in the proposal. Could you please review it?
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, I shall tomorrow when I'm back from holiday.
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.
Okay, I'll close this for now as requested until the proposal is passed. I'll reopen it once we are ready
Update `expectFailure` option to accept a string message and display it in the TAP reporter output. Example output: `# EXPECTED FAILURE <reason>`
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`.
Update expectFailure to accept different types of values (RegExp, Function, Object) for error validation. This change introduces a more flexible API: - String: Acts as a failure label. - Matcher (RegExp, Function, Error): Validates the thrown error. - Object: Supports both 'label' and 'match' properties.
061f049 to
346ec8f
Compare
Summary
This PR enhances the
expectFailureoption in the test runner to accept different types of values, enabling both custom failure labels and robust error validation. This implementation is referenced from and inspired by nodejs/test-runner#10.Changes
The
expectFailureoption now supports the following types:assert.throws).labelormatchproperties, it's treated as a configuration object.References