test_runner: support custom message for expectFailure#61563
test_runner: support custom message for expectFailure#61563nodejs-github-bot merged 2 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
@JakobJingleheimer 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?" |
|
@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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61563 +/- ##
==========================================
+ Coverage 88.84% 89.75% +0.91%
==========================================
Files 674 674
Lines 204957 205682 +725
Branches 39309 39438 +129
==========================================
+ Hits 182087 184607 +2520
+ Misses 15088 13316 -1772
+ Partials 7782 7759 -23
🚀 New features to boost your workflow:
|
|
@JakobJingleheimer nope, haven't started this, though I had long ago implemented it in I think it's important to get the requirements nailed. IMHO, #61570. |
|
As I said, let's put together a proposal in the nodejs/test_runner repo 🙂 |
This comment was marked as resolved.
This comment was marked as resolved.
|
reviewed before reading the discussion; imo a string should work as in this PR whether or not it also supports accepting a regex. |
|
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 🙂
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 |
|
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. |
1af3584 to
13aedaa
Compare
|
I've opened a proposal PR in the test-runner repository as suggested by @JakobJingleheimer. |
061f049 to
346ec8f
Compare
vassudanagunta
left a comment
There was a problem hiding this comment.
I think this needs to be documented a little better for the user.
|
@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. |
This comment was marked as resolved.
This comment was marked as resolved.
Actually all I am proposing is that the decision be made "eyes wide open", with implications in full view. While API design is something I care a lot about, I understand this API so it's no skin off my back whichever way it goes. |
This comment was marked as outdated.
This comment was marked as outdated.
Commit Queue failed- Loading data for nodejs/node/pull/61563 ✔ Done loading data for nodejs/node/pull/61563 ----------------------------------- PR info ------------------------------------ Title test_runner: support custom message for expectFailure (#61563) Author sangwook <rewq5991@gmail.com> (@Han5991) Branch Han5991:test-runner/support-getxfail-message -> nodejs:main Labels needs-ci, commit-queue-squash, test_runner Commits 2 - test_runner: expose expectFailure message - test_runner: fix empty object test assert logic Committers 1 - sangwook <rewq5991@gmail.com> PR-URL: https://github.com/nodejs/node/pull/61563 Fixes: https://github.com/nodejs/node/issues/61570 Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Jacob Smith <jacob@frende.me> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/61563 Fixes: https://github.com/nodejs/node/issues/61570 Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Jacob Smith <jacob@frende.me> -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 28 Jan 2026 14:50:42 GMT ✔ Approvals: 3 ✔ - Jordan Harband (@ljharb): https://github.com/nodejs/node/pull/61563#pullrequestreview-3719402319 ✔ - Aviv Keller (@avivkeller): https://github.com/nodejs/node/pull/61563#pullrequestreview-3723610081 ✔ - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/61563#pullrequestreview-3857218091 ✘ Last GitHub CI failed ℹ Last Full PR CI on 2026-02-26T09:15:39Z: https://ci.nodejs.org/job/node-test-pull-request/71464/ - Querying data for job/node-test-pull-request/71464/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/22437607383 |
|
It seems like github ci failed. Should I try running it again with rebase? |
|
CI didn't actually fail though. Earlier this morning I asked one of the CI maintainers; I'm waiting to hear back. |
|
Landed in 2e1265a |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [node](https://nodejs.org) ([source](https://github.com/nodejs/node)) | minor | `25.7.0` → `25.8.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>nodejs/node (node)</summary> ### [`v25.8.0`](https://github.com/nodejs/node/releases/tag/v25.8.0): 2026-03-03, Version 25.8.0 (Current), @​richardlau [Compare Source](nodejs/node@v25.7.0...v25.8.0) ##### Notable Changes - \[[`e55eddea2a`](nodejs/node@e55eddea2a)] - **build, doc**: use new api doc tooling (flakey5) [#​57343](nodejs/node#57343) - \[[`4c181e2277`](nodejs/node@4c181e2277)] - **(SEMVER-MINOR)** **sqlite**: add limits property to DatabaseSync (Mert Can Altin) [#​61298](nodejs/node#61298) - \[[`46ee1eddd7`](nodejs/node@46ee1eddd7)] - **(SEMVER-MINOR)** **src**: add C++ support for diagnostics channels (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`9ddd1a9c27`](nodejs/node@9ddd1a9c27)] - **(SEMVER-MINOR)** **src,permission**: add --permission-audit (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`0d97ec4044`](nodejs/node@0d97ec4044)] - **(SEMVER-MINOR)** **test\_runner**: expose worker ID for concurrent test execution (Ali Hassan) [#​61394](nodejs/node#61394) ##### Commits - \[[`940b58c8c1`](nodejs/node@940b58c8c1)] - **buffer**: optimize buffer.concat performance (Mert Can Altin) [#​61721](nodejs/node#61721) - \[[`0589b0e5a1`](nodejs/node@0589b0e5a1)] - **build**: fix GN for new merve dep (Shelley Vohr) [#​61984](nodejs/node#61984) - \[[`f3d3968dcd`](nodejs/node@f3d3968dcd)] - ***Revert*** "**build**: add temporal test on GHA windows" (Antoine du Hamel) [#​61810](nodejs/node#61810) - \[[`e55eddea2a`](nodejs/node@e55eddea2a)] - **build, doc**: use new api doc tooling (flakey5) [#​57343](nodejs/node#57343) - \[[`b7715292f8`](nodejs/node@b7715292f8)] - **child\_process**: add tracing channel for spawn (Marco) [#​61836](nodejs/node#61836) - \[[`a32a598748`](nodejs/node@a32a598748)] - **crypto**: fix missing nullptr check on RSA\_new() (ndossche) [#​61888](nodejs/node#61888) - \[[`dc384f95b3`](nodejs/node@dc384f95b3)] - **crypto**: fix handling of null BUF\_MEM\* in ToV8Value() (Nora Dossche) [#​61885](nodejs/node#61885) - \[[`3337b095db`](nodejs/node@3337b095db)] - **crypto**: fix potential null pointer dereference when BIO\_meth\_new() fails (Nora Dossche) [#​61788](nodejs/node#61788) - \[[`51ded81139`](nodejs/node@51ded81139)] - **deps**: update undici to 7.22.0 (Node.js GitHub Bot) [#​62035](nodejs/node#62035) - \[[`8aa2fde931`](nodejs/node@8aa2fde931)] - **deps**: update minimatch to 10.2.4 (Node.js GitHub Bot) [#​62016](nodejs/node#62016) - \[[`57dc092eaf`](nodejs/node@57dc092eaf)] - **deps**: upgrade npm to 11.11.0 (npm team) [#​61994](nodejs/node#61994) - \[[`705bbd60a9`](nodejs/node@705bbd60a9)] - **deps**: update simdjson to 4.3.1 (Node.js GitHub Bot) [#​61930](nodejs/node#61930) - \[[`4d411d72e5`](nodejs/node@4d411d72e5)] - **deps**: update acorn-walk to 8.3.5 (Node.js GitHub Bot) [#​61928](nodejs/node#61928) - \[[`f53a32ab84`](nodejs/node@f53a32ab84)] - **deps**: update acorn to 8.16.0 (Node.js GitHub Bot) [#​61925](nodejs/node#61925) - \[[`9b483fbb27`](nodejs/node@9b483fbb27)] - **deps**: update minimatch to 10.2.2 (Node.js GitHub Bot) [#​61830](nodejs/node#61830) - \[[`4e54c103cb`](nodejs/node@4e54c103cb)] - **doc**: separate in-types and out-types in SQLite conversion docs (René) [#​62034](nodejs/node#62034) - \[[`ca78ebbeaa`](nodejs/node@ca78ebbeaa)] - **doc**: fix small logic error in DETECT\_MODULE\_SYNTAX (René) [#​62025](nodejs/node#62025) - \[[`e6b131f3fe`](nodejs/node@e6b131f3fe)] - **doc**: fix module.stripTypeScriptTypes indentation (René) [#​61992](nodejs/node#61992) - \[[`7508540e19`](nodejs/node@7508540e19)] - **doc**: update DEP0040 (punycode) to application type deprecation (Mike McCready) [#​61916](nodejs/node#61916) - \[[`33a364cb62`](nodejs/node@33a364cb62)] - **doc**: explicitly mention Slack handle (Rafael Gonzaga) [#​61986](nodejs/node#61986) - \[[`46a61922bd`](nodejs/node@46a61922bd)] - **doc**: support toolchain Visual Studio 2022 & 2026 + Windows 11 SDK (Mike McCready) [#​61864](nodejs/node#61864) - \[[`dc12a257aa`](nodejs/node@dc12a257aa)] - **doc**: rename invalid `function` parameter (René) [#​61942](nodejs/node#61942) - \[[`dafdc0a5b8`](nodejs/node@dafdc0a5b8)] - **http**: validate headers in writeEarlyHints (Richard Clarke) [#​61897](nodejs/node#61897) - \[[`3c94b56fa6`](nodejs/node@3c94b56fa6)] - **inspector**: unwrap internal/debugger/inspect imports (René) [#​61974](nodejs/node#61974) - \[[`8a24c17648`](nodejs/node@8a24c17648)] - **lib**: improve argument handling in Blob constructor (Ms2ger) [#​61980](nodejs/node#61980) - \[[`21d4baf256`](nodejs/node@21d4baf256)] - **meta**: bump github/codeql-action from 4.32.0 to 4.32.4 (dependabot\[bot]) [#​61911](nodejs/node#61911) - \[[`59a726a8e3`](nodejs/node@59a726a8e3)] - **meta**: bump step-security/harden-runner from 2.14.1 to 2.14.2 (dependabot\[bot]) [#​61909](nodejs/node#61909) - \[[`0072b7f991`](nodejs/node@0072b7f991)] - **meta**: bump actions/stale from 10.1.1 to 10.2.0 (dependabot\[bot]) [#​61908](nodejs/node#61908) - \[[`999bf22f47`](nodejs/node@999bf22f47)] - **repl**: keep reference count for `process.on('newListener')` (Anna Henningsen) [#​61895](nodejs/node#61895) - \[[`4c181e2277`](nodejs/node@4c181e2277)] - **(SEMVER-MINOR)** **sqlite**: add limits property to DatabaseSync (Mert Can Altin) [#​61298](nodejs/node#61298) - \[[`aee2a18257`](nodejs/node@aee2a18257)] - **src**: fix flags argument offset in JSUdpWrap (Weixie Cui) [#​61948](nodejs/node#61948) - \[[`46ee1eddd7`](nodejs/node@46ee1eddd7)] - **(SEMVER-MINOR)** **src**: add C++ support for diagnostics channels (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`9ddd1a9c27`](nodejs/node@9ddd1a9c27)] - **(SEMVER-MINOR)** **src,permission**: add --permission-audit (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`ea2df2a16f`](nodejs/node@ea2df2a16f)] - **stream**: fix pipeTo to defer writes per WHATWG spec (Matteo Collina) [#​61800](nodejs/node#61800) - \[[`aa0c7b09e0`](nodejs/node@aa0c7b09e0)] - **test**: remove unnecessary `process.exit` calls from test files (Antoine du Hamel) [#​62020](nodejs/node#62020) - \[[`ad96a6578f`](nodejs/node@ad96a6578f)] - **test**: skip `test-url` on `--shared-ada` builds (Antoine du Hamel) [#​62019](nodejs/node#62019) - \[[`7c72a31e4b`](nodejs/node@7c72a31e4b)] - **test**: skip strace test with shared openssl (Richard Lau) [#​61987](nodejs/node#61987) - \[[`604456c163`](nodejs/node@604456c163)] - **test**: avoid flaky debugger restart waits (Yuya Inoue) [#​61773](nodejs/node#61773) - \[[`4890d6bd43`](nodejs/node@4890d6bd43)] - **test\_runner**: run afterEach on runtime skip (Igor Shevelenkov) [#​61525](nodejs/node#61525) - \[[`fce2930110`](nodejs/node@fce2930110)] - **test\_runner**: expose expectFailure message (sangwook) [#​61563](nodejs/node#61563) - \[[`0d97ec4044`](nodejs/node@0d97ec4044)] - **(SEMVER-MINOR)** **test\_runner**: expose worker ID for concurrent test execution (Ali Hassan) [#​61394](nodejs/node#61394) - \[[`243e6b2009`](nodejs/node@243e6b2009)] - **test\_runner**: replace native methods with primordials (Ayoub Mabrouk) [#​61219](nodejs/node#61219) - \[[`bf1ed7e647`](nodejs/node@bf1ed7e647)] - **tls**: forward keepAlive, keepAliveInitialDelay, noDelay to socket (Sergey Zelenov) [#​62004](nodejs/node#62004) - \[[`0f15079d94`](nodejs/node@0f15079d94)] - **tools**: remove custom logic for skipping `test-strace-openat-openssl` (Antoine du Hamel) [#​62038](nodejs/node#62038) - \[[`54a055a59d`](nodejs/node@54a055a59d)] - **tools**: bump minimatch from 3.1.2 to 3.1.3 in `/tools/clang-format` (dependabot\[bot]) [#​61977](nodejs/node#61977) - \[[`a28744cb62`](nodejs/node@a28744cb62)] - **tools**: fix permissions for merve update script (Richard Lau) [#​62023](nodejs/node#62023) - \[[`31e7936354`](nodejs/node@31e7936354)] - **tools**: revert tools GHA workflow to ubuntu-latest (Richard Lau) [#​62024](nodejs/node#62024) - \[[`0a96a16e1f`](nodejs/node@0a96a16e1f)] - **tools**: bump minimatch from 3.1.2 to 3.1.3 in /tools/eslint (dependabot\[bot]) [#​61976](nodejs/node#61976) - \[[`f279233412`](nodejs/node@f279233412)] - **tools**: roll back to x86 runner on `scorecard.yml` (Antoine du Hamel) [#​61944](nodejs/node#61944) - \[[`192c0382f4`](nodejs/node@192c0382f4)] - **util**: add fast path to stripVTControlCharacters (Hiroki Osame) [#​61833](nodejs/node#61833) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My40OS4wIiwidXBkYXRlZEluVmVyIjoiNDMuNDkuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6Om1pbm9yIl19-->
Summary
This PR enhances the
expectFailureoption 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
expectFailureoption now supports the following types:String: Treated as a failure label (reason).
RegExp / Function / Error Class: Treated as a matcher to validate the thrown error (similar to
assert.throws).Object:
labelormatchproperties, it's treated as a configuration object.Inheritance:
expectFailurefrom their parent suite. This allows marking an entire suite as expected to fail.References
expectFailurelabel and/or matcher test-runner#10Resolves: #61570