assert: use Same-value equality in deepStrictEqual #15398
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BridgeAR
added
assert
nodejs-github-bot
added
the
assert
jasnell
approved these changes
Sep 13, 2017
cjihrig
approved these changes
Sep 13, 2017
addaleax
approved these changes
Sep 14, 2017
Trott
approved these changes
Sep 14, 2017
doc/api/assert.md
Outdated
| @@ -110,6 +110,9 @@ parameter is an instance of an `Error` then it will be thrown instead of the | |||
| added: v1.2.0 | |||
| changes: | |||
| - version: REPLACEME | |||
| pr-url: https://github.com/nodejs/node/pull/REPLACEME | |||
| description: zero is now compared using the [Object.is][] comparison. | |||
There was a problem hiding this comment.
Nit: The fact that Object.is() is used under the hood is probably not of interest to the end user compared to what the impact is for the end user. Like, if the big change is that +0 and -0 are no longer considered equal, maybe let's say that instead?
lib/assert.js
Outdated
| @@ -118,12 +118,13 @@ function areSimilarRegExps(a, b) { | |||
| // For small buffers it's faster to compare the buffer in a loop. The c++ | |||
| // barrier including the Uint8Array operation takes the advantage of the faster | |||
| // binary compare otherwise. The break even point was at about 300 characters. | |||
| function areSimilarTypedArrays(a, b) { | |||
| // NOTE: Floats should only use binary comparison in non strict mode! | |||
There was a problem hiding this comment.
Nit: Will this comment be clear to implementers? Would it be better to remove it and let tests catch any errors introduced by maintainers?
There was a problem hiding this comment.
Sure, I removed the comment and the tests already include cases for this, so there is nothing to add anymore.
|
Landed in b0d3bec |
BridgeAR
added a commit
that referenced
this pull request
Sep 16, 2017
PR-URL: #15398 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax
pushed a commit
to addaleax/ayo
that referenced
this pull request
Sep 17, 2017
PR-URL: nodejs/node#15398 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
Qard
pushed a commit
to Qard/ayo
that referenced
this pull request
Sep 21, 2017
PR-URL: nodejs/node#15398 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
decareano
added a commit
to decareano/node
that referenced
this pull request
Sep 22, 2017
doc: fix http.ClientRequest method descriptions fix documentation for methods getHeader, setHeader and removeHeader for http.ClientRequest class. The documentation said these functions can be called but they're wasn't describe into the API description yet. add parameters and general description for each methods. PR-URL: nodejs#15163 Fixes: nodejs#15048 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> assert: use Same-value equality in deepStrictEqual PR-URL: nodejs#15398 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> formatting fix and scar deleted
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is one of the last changes I think is necessary to make
assert.deepStrictEqualreally great.As a side-effect I also improved the performance for Float*Array immensely. It depends on the size of the TypedArray but especially unequal ones will be found much, much faster now.
I also aligned Float*Array with other
TypedArrays inassert.deepEqual. This was not the case and it was tested stricter before (as in testing for extra properties).Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
assert