doc(proposal): add expectFailure enhancements

[Han5991]

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.

[vassudanagunta]

@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 expectFailure in line with todo/skip.

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

[Han5991]

@vassudanagunta

Sorry for the confusion. PR #9 was accidentally opened from my other account, so I closed it and opened this one instead.
I have updated this proposal to include the Object support (both reason and validation) as you suggested in #61570. Thanks for pointing that out!

[vassudanagunta]

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 assert.throws:

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.

[vassudanagunta]

Consistency with assert.throws could be trivial to implement either by sharing code or by calling assert.throws in a try-catch to invert the effect.

[Han5991]

Thanks for the feedback!

Implementation:
Great suggestion! I plan to update the implementation to use assert.throws internally for validation. This will allow us to leverage all the existing validation capabilities (RegExp, Object, Class, custom function) without reinventing the wheel and keep the code cleaner.
API Syntax:
Regarding the flat options (expectFailureError), I lean towards keeping the nested object structure { expectFailure: { with: ..., message: ... } }. It keeps all failure expectations grouped under a single property and seems more extensible if we need to add more configuration in the future, without polluting the top-level options namespace.

[vassudanagunta]

the terseness and legibility of with+message is very nice. as i said on your PR, it gets my top vote.

[JakobJingleheimer]

IIR, we had originally envisioned expectFailure would eventually accept a string for reason, and an object or regex for a matcher:

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(…) }, () => {…});

should do the thing # EXPECT FAILURE "chromium#1234"

should do another thing # EXPECT FAILURE matching /some telltale needle/

should do something else # EXPECT FAILURE matching RangeError(…)

Where matcher would follow the behaviour of assert.throws (and would probably leverage it under the hood).

[Han5991]

IIR, we had originally envisioned expectFailure would eventually accept a string for reason, and an object or regex for a matcher:

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(…) }, () => {…});

should do the thing # EXPECT FAILURE "chromium#1234"

should do another thing # EXPECT FAILURE matching /some telltale needle/

should do something else # EXPECT FAILURE matching RangeError(…)

Where matcher would follow the behaviour of assert.throws (and would probably leverage it under the hood).

@JakobJingleheimer

I've updated the proposal to reflect the latest feedback. Ready for re-review!

[vassudanagunta]

RE the back and forth on whether expectFailure should be truthy, I think it should be an open question to discuss, either with input from other voices here in comments or in the test runner team meeting. Here is my opinion copied from the PR:

I agree with @ljharb in general, but in this specific case it will result in assertFailure behaving differently from todo and skip (see the resolved comment). I think that will be odd/confusing.

Also, given the specificity of the linked todo/skip logic, that behavior seems rather intentional. 🤷🏾‍♂️

[JakobJingleheimer]

Thanks for this!

I think some whitespace is amiss (e.g. too many spaces between * and the text, too much indentation, missing blank line between heading and its content, etc).

[Han5991]

Thanks for the detailed review! I've updated the proposal to address your feedback

[JakobJingleheimer]

🙌 LGTM aside from the small revert needed because of my mistaken assumption.

[ljharb]

LGTM overall

[Han5991]

Thanks for the feedback. Changes have been applied based on your comments.

[vassudanagunta]

Better to be consistent with assert.throws?

[vassudanagunta]

@JakobJingleheimer et. al., when writing my suggested changes to the PR, I noticed assert.throws names this field error, not match. Might it be better to be consistent?

[JakobJingleheimer]

Ah, sure, yes :) error was one of the ideas I suggested (but without knowing it was what assert.throws uses).