test_runner: support custom message for expectFailure

[Han5991]

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

• Inheritance:

  • Tests now inherit expectFailure from their parent suite. This allows marking an entire suite as expected to fail.

    test('suite', { expectFailure: true }, async (t) => {
      await t.test('subtest', () => { throw new Error(); }); // Passes (expected failure)
    });

References

• Based on concepts from: doc(proposal): expectFailure label and/or matcher test-runner#10

Resolves: #61570

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[JakobJingleheimer]

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

[Han5991]

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]

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

Codecov Report

❌ Patch coverage is 87.95181% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.71%. Comparing base (da5efc4) to head (4c7bb37).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/test_runner/test.js	87.80%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61563      +/-   ##
==========================================
+ Coverage   88.84%   89.71%   +0.87%     
==========================================
  Files         674      672       -2     
  Lines      204957   204601     -356     
  Branches    39309    39290      -19     
==========================================
+ Hits       182087   183556    +1469     
+ Misses      15088    13364    -1724     
+ Partials     7782     7681     -101     

Files with missing lines	Coverage Δ
lib/internal/test_runner/reporter/tap.js	94.88% <100.00%> (ø)
lib/internal/test_runner/test.js	96.85% <87.80%> (-0.49%)	⬇️

... and 119 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]

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

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

[ljharb]

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

[JakobJingleheimer]

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]

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]

@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

[vassudanagunta]

I think this needs to be documented a little better for the user.

[JakobJingleheimer]

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

[vassudanagunta]

Looks like suite-level support for expectFailure was missed the first time around. Should fix this now... I think it will be easy.

[vassudanagunta]

good work!

[Han5991]

@JakobJingleheimer

The PR is ready. Please review it!

[vassudanagunta]

We still have the question about whether todo, skip and expectFailure should be truthy. Probably should be tackled separately from this PR, so I opened an issue: #61815

@JakobJingleheimer @ljharb

[JakobJingleheimer]

Awesome—thanks for this 🙌

Just the primordial is a "change needed"; you can take or leave the nits.

[JakobJingleheimer]

nit:

Suggested change

	const message = typeof this.expectFailure === 'object' ?
	this.expectFailure.label :
	this.expectFailure;
	const message = this.expectFailure?.label || this.expectFailure;

[Han5991]

If we use || this.expectFailure, the message variable incorrectly becomes an object (e.g., { label: undefined, ... }) instead of a string or undefined. This happens because this.expectFailure is internally normalized to an object even for boolean inputs. The test runner expects a string message, and passing an object causes serialization errors.

[JakobJingleheimer]

Ohhh 👍 That's weird 🤔

[vassudanagunta]

These sorts of implementation subtleties are prone to unintended behavioral changes as the code evolves. I suggest having more corner case coverage to lock down ambiguities and facilitate further review, e.g. #61563 (review)

[JakobJingleheimer]

Oh, actually, that's a good point. I think we can avoid any logic here by making this.expectFailure adhere to

either

type ExpectFailure = false | {
  label: string, // empty string when user did not provide
  matcher: Matcher | nullish,
}

or

type ExpectFailure = false | {
  label?: string,
  match?: Matcher,
}

Regardless of how the user provided label and/or match, always set them internally to this.expectFailure.label and this.expectFailure.match.

Ex

test.expectFailure('whatever', () => {…});

this.expectFailure = {};

---

test('whatever', { expectFailure: '#42' }, () => {…});

this.expectFailure = { label: '#42' };

---

test('whatever', { expectFailure: { label: '#42' } }, () => {…});

this.expectFailure = { label: '#42' };

[Han5991]

Awesome—thanks for this 🙌

Just the primordial is a "change needed"; you can take or leave the nits.

Thank you for your review. We have updated the update to reflect your requests.

[vassudanagunta]

How should semantic ambiguities in corner case expectFailure: <object> values be interpreted? Both for behavioral stability¹ and to facilitate discussion of desired behavior, I suggest more tests cases such as this one.

Footnotes

1. Behavior is prone to instability as interpretation is distributed in many places, e.g. in parseExpectFailure(expectFailure) and in https://github.com/Han5991/node/blob/5465c1cf3eb2984aaf9918a65c89bc9125ac74a1/lib/internal/test_runner/test.js#L1437-L1442, and also because the logic can be subtle: https://github.com/nodejs/node/pull/61563#discussion_r2815755827 ↩

[Han5991]

How should semantic ambiguities in corner case expectFailure: <object> values be interpreted? Both for behavioral stability1 and to facilitate discussion of desired behavior, I suggest more tests cases such as this one.

Footnotes

1. Behavior is prone to instability as interpretation is distributed in many places, e.g. in parseExpectFailure(expectFailure) and in https://github.com/Han5991/node/blob/5465c1cf3eb2984aaf9918a65c89bc9125ac74a1/lib/internal/test_runner/test.js#L1437-L1442, and also because the logic can be subtle: test_runner: support custom message for expectFailure #61563 (comment) ↩

I agree with your opinion.

[JakobJingleheimer]

@Han5991 please stop force-pushing unless it's absolutely necessary; it totally screws up GitHub's review interface, making it impossible to see a diff of incremental changes (so the entire thing has to get reviewed again, including unchanged lines).

The branch will get collapsed into a single commit automatically on merge, so no need to worry that :)

[Han5991]

@Han5991 please stop force-pushing unless it's absolutely necessary; it totally screws up GitHub's review interface, making it impossible to see a diff of incremental changes (so the entire thing has to get reviewed again, including unchanged lines).

The branch will get collapsed into a single commit automatically on merge, so no need to worry that :)

Thanks for the advice. I'll try not to force pushes.

[JakobJingleheimer]

Nice new test cases—thanks! 🙂

LMK if you want to address the nit or leave it. If leave, I'll land this now and address in a little follow-up.

[JakobJingleheimer]

@Han5991 thanks for jumping on the nit! 🙌

I just realised https://github.com/nodejs/node/pull/61563/changes#r2849591705 got closed before the discussion was settled.

Sorry this is a bit of a never ending story 😖 If you're tired of all the back and forth and if my suggestion adequately addresses the raised concern, I think that can be handled in a follow-up (I'm happy to handle if you don't want to) since it's implementation details that don't affect the public API.

But let's give @vassudanagunta a bit of time to read and respond (and hopefully confirm) before jumping on an update if that is handled within this PR.

[Han5991]

@JakobJingleheimer

I totally agree with your suggestion. Since it doesn't affect the public API, please feel free to merge this PR now. I’m happy to let you handle the implementation details in a follow-up as you offered. Thanks for taking it from here!