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

assert: show the diff when deep comparing data with a custom message #54759

Conversation

puskin94
Copy link
Contributor

@puskin94 puskin94 commented Sep 4, 2024

Fixes: #48465

This PR allows some assertion methods to show the diff when 2 data are different and a custom error message is passed

From this:

image

To this:

image

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. labels Sep 4, 2024
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.61%. Comparing base (5949e16) to head (0d996bc).
Report is 326 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #54759   +/-   ##
=======================================
  Coverage   87.60%   87.61%           
=======================================
  Files         650      650           
  Lines      182829   182836    +7     
  Branches    35379    35381    +2     
=======================================
+ Hits       160173   160185   +12     
+ Misses      15928    15926    -2     
+ Partials     6728     6725    -3     
Files with missing lines Coverage Δ
lib/internal/assert/assertion_error.js 100.00% <100.00%> (ø)

... and 31 files with indirect coverage changes

@puskin94 puskin94 force-pushed the show-diff-in-comparison-with-custom-message branch from 89cb67f to 0d996bc Compare September 4, 2024 15:04
@pmarchini
Copy link
Member

pmarchini commented Sep 6, 2024

@RedYetiDev, do you think we should tag someone or ask for a review from the assert/test group?

Edit: Maybe @MoLow would be interested in reviewing this (I saw this #48465 (comment))

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@RedYetiDev
Copy link
Member

CC @nodejs/assert

@puskin94
Copy link
Contributor Author

Any news on this? is this ready to land?

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2024
@nodejs-github-bot

This comment was marked as outdated.

@puskin94
Copy link
Contributor Author

The PR is ready but the CI is flaky

@nodejs-github-bot
Copy link
Collaborator

jasnell pushed a commit that referenced this pull request Sep 28, 2024
Fixes: #48465
PR-URL: #54759
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Sep 28, 2024

Landed in e1d8b4f

@jasnell jasnell closed this Sep 28, 2024
targos pushed a commit that referenced this pull request Oct 4, 2024
Fixes: #48465
PR-URL: #54759
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 4, 2024
Fixes: #48465
PR-URL: #54759
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
Fixes: nodejs#48465
PR-URL: nodejs#54759
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show diff if test fails with an assertion that has a message
8 participants