-
-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add expectFailure enhancements proposal #10
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
|
@Han5991 this should reference nodejs/node#61570 which goes beyond nodejs/node#61563. It may end up being the case that a fix for the latter lands before the former, since the latter is relatively trivial and simply brings --EDIT-- I'm a little confused. Your first proposal, #9, included a way to supply both a reason and an expected error as I proposed in nodejs/node#61570, but this one does not. |
…1570 requirement)
|
Sorry for the confusion. PR #9 was accidentally opened from my other account, so I closed it and opened this one instead. |
|
Alternative syntaxes to consider for supplying both reason and expected error: test('fails with reason and specific error', {
expectFailure: 'Bug #123: Edge case behavior',
expectFailureMessage: /err msg pattern/
}, () => ...);or for consistency with test('fails with reason and specific error', {
expectFailure: 'Bug #123: Edge case behavior',
expectFailureError: <RegExp> | <Function> | <Object> | <Error>
}, () => ...);The user can supply either one of the two new options or both. |
|
Consistency with |
|
Thanks for the feedback! Implementation: |
|
the terseness and legibility of with+message is very nice. as i said on your PR, it gets my top vote. |
Update the `expectFailure` enhancements proposal based on feedback and implementation alignment:
- Consolidate validation logic under the `with` property within an object.
- Remove direct RegExp support in favor of the object syntax for consistency.
- Specify usage of `assert.throws` for robust error validation.
- Document alternatives considered (flat options).
|
IIR, we had originally envisioned it('should do the thing', { expectFailure: 'chromium#1234' }, () => {…});
it('should do another thing', { expectFailure: /some telltale needle/ }, () => {…});
it('should do something else', { expectFailure: new RangeError(…) }, () => {…});Where matcher would follow the behaviour of |
0de23a6 to
abb5912
Compare
| ## Edge Cases & Implementation Details | ||
|
|
||
| ### Empty String (`expectFailure: ''`) | ||
| Following standard JavaScript truthiness rules, an empty string should be treated as **falsy**. |
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 far more important to be consistent with todo/skip, for which an empty string is the same as true: https://github.com/nodejs/node/blob/68d7b6f87098416dc30fbb711fdff64fc2102809/lib/internal/test_runner/test.js#L639-L640
If there is a desire to use truthiness, then todo/skip should also be changed. This might break some clients, but given the odd use, it would be rare and ok to expect them to fix their usage as long as it goes with at least a Node version SemVer minor change and a release warning.
This PR adds a proposal for enhancing expectFailure to support both custom messages (for reasoning) and validation (matching errors via regex/objects), addressing the needs discussed in nodejs/node#61563.