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: fix the string length check for printing the simple diff #55474

Merged

Conversation

puskin94
Copy link
Contributor

Fixes a small issue introduced in #54862

The check that was comparing the total string length to decide if the comparison was long enough to pick between the "stacked" diff or the "simple" one, was not considering the double quotes surrounding actual and expected when they are strings

@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 Oct 20, 2024
@puskin94 puskin94 changed the title assert: fix the string length check for printing the simple diff Draft: assert: fix the string length check for printing the simple diff Oct 20, 2024
Copy link

codecov bot commented Oct 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.42%. Comparing base (b0ffe9e) to head (228cc94).
Report is 89 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55474      +/-   ##
==========================================
- Coverage   88.42%   88.42%   -0.01%     
==========================================
  Files         653      653              
  Lines      187498   187422      -76     
  Branches    36100    36072      -28     
==========================================
- Hits       165791   165721      -70     
+ Misses      14957    14950       -7     
- Partials     6750     6751       +1     
Files with missing lines Coverage Δ
lib/internal/assert/assertion_error.js 95.22% <100.00%> (+0.08%) ⬆️

... and 42 files with indirect coverage changes

@RedYetiDev RedYetiDev added the wip Issues and PRs that are still a work in progress. label Oct 20, 2024
@RedYetiDev
Copy link
Member

Hi! Please mark the PRas draft if it's not ready to be reviewed, thank way, it'll be easier for reviewers to peruse the issue tracker

@jasnell jasnell marked this pull request as draft October 20, 2024 22:59
@puskin94 puskin94 force-pushed the fix-length-simple-assert-diff branch from 02d82f2 to 228cc94 Compare October 21, 2024 08:44
@puskin94 puskin94 changed the title Draft: assert: fix the string length check for printing the simple diff assert: fix the string length check for printing the simple diff Oct 21, 2024
@puskin94 puskin94 marked this pull request as ready for review October 21, 2024 08:44
@puskin94
Copy link
Contributor Author

puskin94 commented Oct 21, 2024

Hi! Please mark the PRas draft if it's not ready to be reviewed, thank way, it'll be easier for reviewers to peruse the issue tracker

Yes, yesterday when I pushed I was running aroung in between flights, it was hard for me to handle github on the phone :)

You can remove the wip label now!

@pmarchini pmarchini removed the wip Issues and PRs that are still a work in progress. label Oct 21, 2024
@pmarchini
Copy link
Member

Hey @puskin94, I think we should also add some tests related to the difference in behavior between "colored" and "non-colored" diff.
There's a PR that is partially related: #55404, I would suggest waiting for it to land before adding the tests, as the Myers diff leverages colors.hasColors (if I'm not mistaken)

@puskin94
Copy link
Contributor Author

@pmarchini that MR has been merged but didn't improve the colored output in assertion related checks

@pmarchini
Copy link
Member

@pmarchini that MR has been merged but didn't improve the colored output in assertion related checks

Hey @puskin94, could you please create a draft PR containing some tests to highlight the unexpected behaviors?
The PR itself changed the way hasColors works.
Before the PR, many cases were just not using colors even when expected, which probably made some issues evident, or at least I would say so.
The fact that no tests broke following the changes also highlights that we need to cover that specific subset of use cases.
Would you like to try investigating the issue? 😬

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

@pmarchini
Copy link
Member

FYI @nodejs/assert @nodejs/test_runner

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2024
@nodejs-github-bot
Copy link
Collaborator

@pmarchini pmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 28, 2024
@marco-ippolito marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 2, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 2, 2024
@nodejs-github-bot nodejs-github-bot merged commit 10cce65 into nodejs:main Nov 2, 2024
83 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 10cce65

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#55474
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
aduh95 pushed a commit that referenced this pull request Nov 5, 2024
PR-URL: #55474
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants