Skip to content

Conversation

@Han5991
Copy link
Contributor

@Han5991 Han5991 commented Jan 28, 2026

Summary

This PR enhances the expectFailure option 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 expectFailure option now supports the following types:

  • String: Treated as a failure label (reason).
    test('bug', { expectFailure: 'Investigating' }, ...);
  • RegExp / Function / Error Class: Treated as a matcher to validate the thrown error (similar to assert.throws).
    test('regex', { expectFailure: /error message/ }, ...);
    test('class', { expectFailure: RangeError }, ...);
  • Object:
    • If it contains label or match properties, it's treated as a configuration object.
      test('config', {
        expectFailure: {
          label: 'Known issue',
          match: /specific error/
        }
      }, ...);
    • Otherwise, it's treated as an object matcher (properties matching).

References

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jan 28, 2026
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a 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.

@Han5991
Copy link
Contributor Author

Han5991 commented Jan 28, 2026

We already gave a plan for the value of expectFail: it will soon accept a regular expression to match against.

@JakobJingleheimer
Ah, thanks for the context! I missed that there was an existing plan for error matching.

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?"

@JakobJingleheimer
Copy link
Member

@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
Copy link

codecov bot commented Jan 28, 2026

Codecov Report

❌ Patch coverage is 97.43590% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.75%. Comparing base (3819c7f) to head (7145647).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/test_runner/test.js 97.40% 2 Missing ⚠️
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     
Files with missing lines Coverage Δ
lib/internal/test_runner/reporter/tap.js 98.23% <100.00%> (ø)
lib/internal/test_runner/test.js 96.84% <97.40%> (-0.49%) ⬇️

... and 43 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vassudanagunta
Copy link
Contributor

@JakobJingleheimer nope, haven't started this, though I had long ago implemented it in node-test-extra (not yet released).

I think it's important to get the requirements nailed. IMHO, #61570.

@JakobJingleheimer
Copy link
Member

As I said, let's put together a proposal in the nodejs/test_runner repo 🙂

@vassudanagunta
Copy link
Contributor

As I said

?

I should start a discussion in that repo?

@ljharb
Copy link
Member

ljharb commented Jan 28, 2026

reviewed before reading the discussion; imo a string should work as in this PR whether or not it also supports accepting a regex.

@JakobJingleheimer
Copy link
Member

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 🙂


I should start a discussion in that repo?

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

@ljharb
Copy link
Member

ljharb commented Jan 28, 2026

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.

@Han5991
Copy link
Contributor Author

Han5991 commented Jan 29, 2026

@ljharb @vassudanagunta

I've opened a proposal PR in the test-runner repository as suggested by @JakobJingleheimer.
It would be great to continue the discussion on the spec details there:
nodejs/test-runner#10

Comment on lines 254 to 329
expectFailure: {
with: /error message/,
message: 'reason for failure',
},

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Contributor Author

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 with is for validation and message is for reasoning.
  • Noted that expectFailure: 'reason' and { message: 'reason' } are equivalent.

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Member

@JakobJingleheimer JakobJingleheimer Jan 31, 2026

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).

Copy link
Contributor Author

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?

nodejs/test-runner#10

Copy link
Member

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.

Copy link
Contributor Author

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

@Han5991 Han5991 marked this pull request as draft January 31, 2026 07:56
@Han5991 Han5991 closed this Jan 31, 2026
@Han5991 Han5991 reopened this Feb 3, 2026
@Han5991 Han5991 marked this pull request as ready for review February 3, 2026 22:45
@Han5991 Han5991 force-pushed the test-runner/support-getxfail-message branch from 061f049 to 346ec8f Compare February 4, 2026 00:49
Copy link
Contributor

@vassudanagunta vassudanagunta left a 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
Copy link
Contributor

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:

Suggested change
In each of the following, `doTheThing()` fails to return `true`,

doc/api/test.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
but since the tests are flagged `expectFailure`, they pass.

it('should do the thing', { expectFailure: 'feature not implemented' }, () => {
assert.strictEqual(doTheThing(), true);
});

Copy link
Contributor

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.

Suggested change
```
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', {
Copy link
Contributor

@vassudanagunta vassudanagunta Feb 4, 2026

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
Copy link
Contributor

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.

@JakobJingleheimer
Copy link
Member

JakobJingleheimer commented Feb 4, 2026

@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.

@Han5991 Han5991 marked this pull request as draft February 4, 2026 21:42
@Han5991
Copy link
Contributor Author

Han5991 commented Feb 4, 2026

Thanks for letting me know. I'll leave it as a draft until the proposal is finalized.

@JakobJingleheimer JakobJingleheimer dismissed their stale review February 9, 2026 20:03

Resolved by proposal

@JakobJingleheimer
Copy link
Member

@Han5991 the proposal is all set, so full-steam ahead 🙂 (or whatever pace you want)

@Han5991 Han5991 force-pushed the test-runner/support-getxfail-message branch from 7946173 to 48e85f0 Compare February 9, 2026 22:30
@Han5991 Han5991 marked this pull request as ready for review February 9, 2026 22:30
@Han5991 Han5991 force-pushed the test-runner/support-getxfail-message branch from 48e85f0 to a5eaf98 Compare February 9, 2026 22:32
@Han5991
Copy link
Contributor Author

Han5991 commented Feb 9, 2026

@JakobJingleheimer
Thank you for taking the time to let me know!

@Han5991 Han5991 force-pushed the test-runner/support-getxfail-message branch from a5eaf98 to c2bebdf Compare February 9, 2026 22:43
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.
@Han5991 Han5991 force-pushed the test-runner/support-getxfail-message branch from c2bebdf to 7145647 Compare February 10, 2026 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants