Skip to content
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

test_runner: add assert.register() API #56434

Merged
merged 2 commits into from
Jan 4, 2025
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 2, 2025

This commit adds a top level assert.register() API to the test runner. This function allows users to define their own custom assertion functions on the TestContext.

Fixes: #52033

@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 2, 2025
doc/api/test.md Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.53%. Comparing base (afafee2) to head (968cbbf).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56434      +/-   ##
==========================================
- Coverage   88.53%   88.53%   -0.01%     
==========================================
  Files         657      658       +1     
  Lines      190761   190807      +46     
  Branches    36616    36619       +3     
==========================================
+ Hits       168899   168939      +40     
+ Misses      15048    15044       -4     
- Partials     6814     6824      +10     
Files with missing lines Coverage Δ
lib/internal/test_runner/assert.js 100.00% <100.00%> (ø)
lib/internal/test_runner/test.js 96.92% <100.00%> (-0.04%) ⬇️
lib/test.js 100.00% <100.00%> (ø)

... and 26 files with indirect coverage changes

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.

LGTM :)

lib/internal/test_runner/test.js Show resolved Hide resolved
lib/test.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@pmarchini pmarchini left a comment

Choose a reason for hiding this comment

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

LGTM, however I have only one doubt regarding whether it will fix the related issue (#52033).

@cjihrig cjihrig added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 3, 2025
This commit adds a top level assert.register() API to the test
runner. This function allows users to define their own custom
assertion functions on the TestContext.

Fixes: nodejs#52033
@cjihrig cjihrig added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 3, 2025
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 3, 2025

LGTM, however I have only one doubt regarding whether it will fix the related issue (#52033).

I think this is as close as we will get to that request. This allows assertions to be created that don't throw. IMO it also doesn't make sense to add logic to node:assert and every other assertion library to send messages to the test runner.

EDIT: Actually, I think we need to add a TestContext.prototype.fail() function in order to have silent assertions. For some reason, I thought that already existed.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 3, 2025

@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jan 4, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 4, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/56434
✔  Done loading data for nodejs/node/pull/56434
----------------------------------- PR info ------------------------------------
Title      test_runner: add assert.register() API (#56434)
Author     Colin Ihrig <[email protected]> (@cjihrig)
Branch     cjihrig:asserts -> nodejs:main
Labels     semver-minor, author ready, commit-queue-squash, test_runner
Commits    2
 - test_runner: add assert.register() API
 - nits
Committers 1
 - cjihrig <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/56434
Fixes: https://github.com/nodejs/node/issues/52033
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Pietro Marchini <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/56434
Fixes: https://github.com/nodejs/node/issues/52033
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Pietro Marchini <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - test_runner: add assert.register() API
   ⚠  - nits
   ℹ  This PR was created on Thu, 02 Jan 2025 13:27:50 GMT
   ✔  Approvals: 3
   ✔  - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/56434#pullrequestreview-2528217838
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/56434#pullrequestreview-2528251814
   ✔  - Pietro Marchini (@pmarchini): https://github.com/nodejs/node/pull/56434#pullrequestreview-2528681830
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-01-03T23:12:12Z: https://ci.nodejs.org/job/node-test-pull-request/64326/
- Querying data for job/node-test-pull-request/64326/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/12610900600

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 4, 2025

This needs another approval or a re-approval in order to land.

@cjihrig cjihrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 4, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 4, 2025
@nodejs-github-bot nodejs-github-bot merged commit 4a7b815 into nodejs:main Jan 4, 2025
77 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 4a7b815

@cjihrig cjihrig deleted the asserts branch January 4, 2025 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. semver-minor PRs that contain new features and should be released in the next minor version. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The assertion module should signals failures through messages to the test runner
6 participants