-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
assert: show diff when doing partial comparisons with a custom message #56211
assert: show diff when doing partial comparisons with a custom message #56211
Conversation
How to show the diff for the partial implementation has been one of my main concerns for a while and my current idea is to grey out or hide all lines that are not in the expected object. That way it would be more straight forward for the user. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56211 +/- ##
==========================================
+ Coverage 88.49% 88.54% +0.04%
==========================================
Files 656 657 +1
Lines 189261 190668 +1407
Branches 36348 36595 +247
==========================================
+ Hits 167493 168833 +1340
- Misses 14977 15019 +42
- Partials 6791 6816 +25
|
e2f640c
to
e4b2307
Compare
e4b2307
to
3db0829
Compare
Is there anything left to be done here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Let's see how people react to the greyed out part to gather feedback while it's experimental. I feel it is nicer than the green plus.
3db0829
to
9f449e9
Compare
9f449e9
to
0018ef3
Compare
Is anything else needed here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
Commit Queue failed- Loading data for nodejs/node/pull/56211 ✔ Done loading data for nodejs/node/pull/56211 ----------------------------------- PR info ------------------------------------ Title assert: show diff when doing partial comparisons with a custom message (#56211) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch puskin94:partial-deep-strict-equal-custom-message-diff -> nodejs:main Labels assert, needs-ci Commits 1 - assert: show diff when doing partial comparisons Committers 1 - Giovanni <[email protected]> PR-URL: https://github.com/nodejs/node/pull/56211 Reviewed-By: Ruben Bridgewater <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/56211 Reviewed-By: Ruben Bridgewater <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 10 Dec 2024 15:24:35 GMT ✔ Approvals: 1 ✔ - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/56211#pullrequestreview-2530930488 ✘ Last GitHub CI failed ℹ Last Full PR CI on 2025-01-06T16:25:01Z: https://ci.nodejs.org/job/node-test-pull-request/64369/ - Querying data for job/node-test-pull-request/64369/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/12638165059 |
Landed in 799d920 |
following the same reasoning as this PR:
right now the method
assert.partialDeepStrictEqual
does not show the diff when the assertion fails.With this PR we go from this:
to this
with a custom error message:
Relevant changes:
assert.partialDeepStrictEqual
failsassert.partialDeepStrictEqual
fails with a custom error messageassert.partialDeepStrictEqual
fails:actual
are grayexpected
but not inactual
are prefixed with the classic red-
actual
andexpected
are white