assert: make partialDeepStrictEqual throw when comparing [0] with [-0] #56237

puskin94 · 2024-12-12T11:00:25Z

assert.partialDeepStrictEqual now throws when comparing [0] with [-0]

codecov · 2024-12-12T12:16:30Z

Codecov Report

Attention: Patch coverage is 94.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.54%. Comparing base (9ec8b9e) to head (cd8e5cd).
Report is 72 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/assert.js	94.00%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #56237    +/-   ##
========================================
  Coverage   88.54%   88.54%            
========================================
  Files         657      657            
  Lines      189899   190426   +527     
  Branches    36465    36565   +100     
========================================
+ Hits       168140   168609   +469     
- Misses      14966    15000    +34     
- Partials     6793     6817    +24

Files with missing lines	Coverage Δ
lib/assert.js	`99.04% <94.00%> (-0.34%)`	⬇️

... and 57 files with indirect coverage changes

test/parallel/test-assert-objects.js

ljharb · 2024-12-12T17:02:28Z

lib/assert.js

+  if (ObjectIs(item, -0)) return '-0(number)';
+  if (item === '-0') return '-0(string)';
+  if (ObjectIs(item, 0)) return '0(number)';
+  if (item === '0') return '0(string)';


can you please add tests that compare between [0] and ['-0(number)'], and every other permutation of the zeroes and these return values?

I feel like using Symbols would be more appropriate than strings.

@ljharb yeah, while I was pushing I thought about the problems my solution had. I approached the problem in a slightly different way and now we should not have those edge cases anymore.

Thanks for the review!

I think using a memoized symbol for -0 would end up being a lot simpler than this implementation

@ljharb would you mind showing me what you mean? The current pushed implementation does not have any hardcoded exception for the 0 case

Ah I see what you mean now! And is this what you would like to see implemented or is it just something that could have been improved the previous implementation, that is no more?

My suspicion is that it would likely be the simplest and fastest implementation, and that it's more similar to your previous implementation than the current one. Your call though!

@ljharb MUCH cleaner implementation indeed, I never think at Symbols, thanks for the hint 😄

(to give proper credit, @LiviaMedeiros suggested it initially upthread :-) )

oh boi, I completely missed the comment! sorry, and thanks both then :)

lib/assert.js

puskin94 · 2024-12-12T19:41:28Z

@ljharb yep, all valid comments. I spent so much time on this finding the best solution and it took everything out of me, making me write dumb stuff... thanks again 😄

nodejs-github-bot · 2024-12-19T12:26:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64117/

nodejs-github-bot · 2024-12-21T06:26:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64145/

LiviaMedeiros

LGTM with the following removal.

LiviaMedeiros · 2024-12-21T15:42:45Z

test/parallel/test-assert-objects.js

+        {
+          description: 'throws when comparing [-0] with [0]',
+          actual: [0],
+          expected: [-0],
+        },


Suggested change

{

description: 'throws when comparing [-0] with [0]',

actual: [0],

expected: [-0],

},

@LiviaMedeiros how come? Why do you think I should remove this?

It’s a duplicate, look two test cases up

Ops, I was checking on the phone, that was tough to spot 😅 the test doesn't have to be removed, the description is right but the test case is wrong. I will fix when home 😊

Fixes: nodejs#56230

nodejs-github-bot · 2024-12-22T10:07:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64148/

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. labels Dec 12, 2024

LiviaMedeiros reviewed Dec 12, 2024

View reviewed changes

test/parallel/test-assert-objects.js Outdated Show resolved Hide resolved

puskin94 marked this pull request as draft December 12, 2024 15:03

puskin94 force-pushed the partial-deep-strict-equal-minus-plus-zero branch from 0ce4a4a to 7c30734 Compare December 12, 2024 15:54

puskin94 marked this pull request as ready for review December 12, 2024 15:55

jasnell approved these changes Dec 12, 2024

View reviewed changes

ljharb reviewed Dec 12, 2024

View reviewed changes

puskin94 force-pushed the partial-deep-strict-equal-minus-plus-zero branch 2 times, most recently from 65e1141 to 4fa9ec7 Compare December 12, 2024 19:06

ljharb reviewed Dec 12, 2024

View reviewed changes

lib/assert.js Outdated Show resolved Hide resolved

lib/assert.js Outdated Show resolved Hide resolved

lib/assert.js Outdated Show resolved Hide resolved

lib/assert.js Outdated Show resolved Hide resolved

lib/assert.js Outdated Show resolved Hide resolved

puskin94 force-pushed the partial-deep-strict-equal-minus-plus-zero branch from 4fa9ec7 to a0d1929 Compare December 12, 2024 19:40

puskin94 force-pushed the partial-deep-strict-equal-minus-plus-zero branch 2 times, most recently from 8f997a5 to 71a5892 Compare December 15, 2024 20:04

jakecastelli added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 19, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2024

LiviaMedeiros removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 21, 2024

LiviaMedeiros reviewed Dec 21, 2024

View reviewed changes

assert: make partialDeepStrictEqual throw when comparing [0] with [-0]

cd8e5cd

Fixes: nodejs#56230

puskin94 force-pushed the partial-deep-strict-equal-minus-plus-zero branch from 71a5892 to cd8e5cd Compare December 21, 2024 20:52

LiviaMedeiros approved these changes Dec 22, 2024

View reviewed changes

LiviaMedeiros added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 22, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: make partialDeepStrictEqual throw when comparing [0] with [-0] #56237

assert: make partialDeepStrictEqual throw when comparing [0] with [-0] #56237

puskin94 commented Dec 12, 2024

codecov bot commented Dec 12, 2024 •

edited

Loading

ljharb Dec 12, 2024

LiviaMedeiros Dec 12, 2024

puskin94 Dec 12, 2024

ljharb Dec 15, 2024

puskin94 Dec 15, 2024

puskin94 Dec 15, 2024

ljharb Dec 15, 2024

puskin94 Dec 15, 2024

ljharb Dec 15, 2024

puskin94 Dec 15, 2024

puskin94 commented Dec 12, 2024 •

edited

Loading

nodejs-github-bot commented Dec 19, 2024

nodejs-github-bot commented Dec 21, 2024

LiviaMedeiros left a comment

LiviaMedeiros Dec 21, 2024

puskin94 Dec 21, 2024

ljharb Dec 21, 2024

puskin94 Dec 21, 2024

nodejs-github-bot commented Dec 22, 2024

assert: make partialDeepStrictEqual throw when comparing [0] with [-0] #56237

Are you sure you want to change the base?

assert: make partialDeepStrictEqual throw when comparing [0] with [-0] #56237

Conversation

puskin94 commented Dec 12, 2024

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puskin94 commented Dec 12, 2024 • edited Loading

nodejs-github-bot commented Dec 19, 2024

nodejs-github-bot commented Dec 21, 2024

LiviaMedeiros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 22, 2024

codecov bot commented Dec 12, 2024 •

edited

Loading

puskin94 commented Dec 12, 2024 •

edited

Loading