-
-
Notifications
You must be signed in to change notification settings - Fork 5
doc(proposal): add expectFailure enhancements #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?
Changes from all commits
036ee9c
049c2d3
abb5912
87802e3
cb12492
550464a
38976f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| # Feature proposal: `expectFailure` enhancements | ||
|
|
||
| ## Summary | ||
Han5991 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Update the `expectFailure` option in `test()` to accept different types of values, enabling both **custom failure messages** and **error validation**. This proposal integrates the requirements from [nodejs/node#61570](https://github.com/nodejs/node/issues/61570), ensuring consistency with `skip`/`todo` while adding robust validation capabilities. | ||
Han5991 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ## API & Behavior | ||
|
|
||
| The behavior of `expectFailure` is strictly determined by the type of value provided: | ||
|
|
||
| ### 1. String: Failure Reason | ||
Han5991 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| When a **non-empty string** is provided, it acts as a documentation message (reason), identical to `skip` and `todo` options. | ||
Han5991 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ```js | ||
| test('fails with a specific reason', { | ||
| expectFailure: 'Bug #123: Feature not implemented yet' | ||
| }, () => { | ||
| throw new Error('boom'); | ||
| }); | ||
| ``` | ||
| - **Behavior**: The test is expected to fail. The string is treated as a label/reason. | ||
| - **Validation**: None. It accepts *any* error. | ||
Han5991 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - **Output**: The reporter displays the string (e.g., `# EXPECTED FAILURE Bug #123...`). | ||
Han5991 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ### 2. Matcher: RegExp, Class, or Error Object | ||
Han5991 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| When a **RegExp**, **Class** (Function), or **Error Object** is provided directly, it acts as the validation logic. This leverages `assert.throws` behavior directly. | ||
Han5991 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ```js | ||
| test('fails with matching error (RegExp)', { | ||
| expectFailure: /expected error message/ | ||
| }, () => { | ||
| throw new Error('this is the expected error message'); | ||
| }); | ||
|
|
||
| test('fails with matching error (Class)', { | ||
| expectFailure: RangeError | ||
| }, () => { | ||
| throw new RangeError('Index out of bounds'); | ||
| }); | ||
| ``` | ||
|
|
||
| ### 3. Configuration Object: Reason & Validation | ||
Han5991 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| When a **Plain Object** with specific properties (`with`, `message`) is provided, it allows specifying both a failure reason and validation logic simultaneously. | ||
|
|
||
| ```js | ||
| test('fails with reason and specific error', { | ||
| expectFailure: { | ||
| message: 'Bug #123: Edge case behavior', // Reason | ||
| with: /Index out of bounds/ // Validation | ||
|
Comment on lines
+47
to
+48
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these keys are a bit ambiguous. I think |
||
| } | ||
| }, () => { | ||
| throw new RangeError('Index out of bounds'); | ||
| }); | ||
| ``` | ||
| - **Properties**: | ||
| - `message` (String): The failure reason/label (displayed in reporter). | ||
| - `with` (RegExp | Object | Function | Class): Validation logic. This is passed directly to `assert.throws` validation argument, supporting all its capabilities. | ||
| - **Behavior**: The test passes **only if** the error matches the `with` criteria. | ||
| - **Output**: The reporter displays the `message`. | ||
|
|
||
| ### Equivalence | ||
| The following configurations are equivalent in behavior: | ||
|
|
||
| **1. Reason only:** | ||
| ```js | ||
| expectFailure: 'reason' | ||
| expectFailure: { message: 'reason' } | ||
| ``` | ||
|
|
||
| **2. Validation only:** | ||
| ```js | ||
| expectFailure: /error/ | ||
| expectFailure: { with: /error/ } | ||
| ``` | ||
|
|
||
| **3. Catch-all (Any Error):** | ||
| ```js | ||
| expectFailure: true | ||
| ``` | ||
|
|
||
| ## Ambiguity Resolution | ||
| Potential ambiguity between a **Matcher Object** and a **Configuration Object** is resolved as follows: | ||
|
|
||
| 1. **String** → Reason. | ||
Han5991 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 2. **RegExp** or **Function** → Matcher (Validation). | ||
| 3. **Object**: | ||
| * **Empty Object** (`{}`) → **Error**: throws `ERR_INVALID_ARG_VALUE`. | ||
| ```js | ||
| // Uses Node.js standard error code | ||
| throw new ERR_INVALID_ARG_VALUE( | ||
| 'expectFailure', | ||
| expectFailure, | ||
| 'must not be an empty object' | ||
| ); | ||
| ``` | ||
| * If the object contains `with` or `message` properties → **Configuration Object**. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if it contains either or both but no more → config object |
||
| * Otherwise → **Matcher Object** (passed to `assert.throws` for property matching). | ||
|
|
||
| ## Activation & Truthiness | ||
| To maintain strict consistency with `todo` and `skip` options: | ||
| * The feature is **disabled** only if `expectFailure` is `undefined` or `false`. | ||
JakobJingleheimer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * **All other values** enable the feature (treat as truthy). | ||
| * `expectFailure: ''` (Empty String) → **Enabled** (treats as generic failure expectation). | ||
| * `expectFailure: 0` → **Enabled** (treated as a Matcher Object unless specific logic excludes numbers, but per consistency it enables the feature). | ||
|
|
||
| ### Flat Options (`expectFailureError`) | ||
| It was proposed to split the options into `expectFailure` (reason) and `expectFailureError` (validation). | ||
| This was rejected in favor of the nested/polymorphic structure using `with` and `message` properties. This syntax was selected as the preferred choice for its readability and clarity: | ||
| * `with`: Clearly indicates "fails **with** this error" (Validation). | ||
| * `message`: Clearly indicates the **reason** or label for the expected failure. | ||
| This approach keeps related configuration grouped without polluting the top-level options namespace. | ||
|
|
||
| ## Implementation Details | ||
|
|
||
| ### Validation Logic | ||
| The implementation leverages `assert.throws` internally to perform error validation. | ||
| - If `expectFailure` is a Matcher (RegExp, Class, Object), it is passed as the second argument to `assert.throws(fn, expectFailure)`. | ||
| - If `expectFailure` is a Configuration Object, `expectFailure.with` is passed to `assert.throws`. | ||
Uh oh!
There was an error while loading. Please reload this page.