-
-
Notifications
You must be signed in to change notification settings - Fork 34.7k
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.75% +0.01%
==========================================
Files 675 675
Lines 204642 204715 +73
Branches 39322 39350 +28
==========================================
+ Hits 183657 183745 +88
+ Misses 13257 13241 -16
- Partials 7728 7729 +1
🚀 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', | ||
| }, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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
061f049 to
346ec8f
Compare
vassudanagunta
left a comment
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 this needs to be documented a little better for the user.
doc/api/test.md
Outdated
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.
That minimum changes to fix the incorrect English is to move "currently" before "returns", and to refer to "test-cases" plural.
But the entire sentence is unnecessarily complex and confusing, needing a rewrite. I'd recommend:
| In each of the following, `doTheThing()` fails to return `true`, |
doc/api/test.md
Outdated
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.
| but since the tests are flagged `expectFailure`, they pass. |
| it('should do the thing', { expectFailure: 'feature not implemented' }, () => { | ||
| assert.strictEqual(doTheThing(), true); | ||
| }); | ||
|
|
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 ability to expect a specific failure deserves some explanation.
| ``` | |
| If the value of `expectFailure` is a | |
| [<RegExp>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp) | | |
| [<Function>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function) | | |
| [<Object>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object) | | |
| [<Error>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error), | |
| the tests will pass only if they throw a matching value. | |
| See [`assert.throws`] for how the each value type is interpreted. | |
| Each of the following tests will fail _despite_ being flagged `expectFailure` | |
| because the failure was not the expected one. | |
| ```js |
You'll need to add a link to the assert.throws documentation at the bottom of the file.
doc/api/test.md
Outdated
| assert.strictEqual(doTheThing(), true); | ||
| }); | ||
|
|
||
| it('should fail with specific error', { |
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.
Finally, I think this example with the {match, label} value should be broken out as the last example with its own explanation, e.g. "To supply both a reason and specific error for assertFailure..."
doc/api/test.md
Outdated
| thread. If `false`, only one test runs at a time. | ||
| If unspecified, subtests inherit this value from their parent. | ||
| **Default:** `false`. | ||
| * `expectFailure` {boolean|string|Object} If truthy, the test is expected to |
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.
Missing: that you can specify a specific error directly, without a wrapping {match: ...} object. And probably should reference the specific types with a link to assert.throws as I suggested above.
If it seems redundant to do that in two places, the I guess provide all the details in this section and link to here from Expecting tests to fail.
|
@Han5991 the proposal isn't finished / accepted yet (still hasn't been reviewed by the rest of the test-runner team), so I think it's premature to resume this (the proposal isn't a requirement, but I think it's a good idea and will reduce churn, needless re-reviews, etc—and indeed, there was just earlier today another adjustment to align terms). I do appreciate the enthusiasm 😁 It's added to the team's agenda, so it'll get raised at the next meeting. |
|
Thanks for letting me know. I'll leave it as a draft until the proposal is finalized. |
|
@Han5991 the proposal is all set, so full-steam ahead 🙂 (or whatever pace you want) |
7946173 to
48e85f0
Compare
48e85f0 to
a5eaf98
Compare
|
@JakobJingleheimer |
a5eaf98 to
c2bebdf
Compare
Normalize expectFailure values to support string labels, direct
matchers, and { label, match } objects while validating errors via
assert.throws.
Update TAP reporting and tests, including function matcher coverage
and unexpected-pass behavior, and clarify docs for direct matcher
usage.
c2bebdf to
7145647
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
expectFailurelabel and/or matcher test-runner#10