assert: adjust loose assertions by BridgeAR · Pull Request #25008 · nodejs/node (original) (raw)

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

BridgeAR

This changes the loose deep equal comparison by using the same logic
as done in the strict deep equal comparison besides comparing
primitives loosely, not comparing symbol properties and not comparing
the prototype.

assert.deepEqual is still commenly used and this is likely the
biggest pitfall.

Most changes are only minor and won't have a big impact besides
likely fixing user expectations.

I guess this might still be a bit bold but I believe it's the right
thing to do. We just have to check how much impact this has in
userland.

Checklist

@BridgeAR BridgeAR added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Dec 13, 2018

@nodejs-github-bot

@BridgeAR

@BridgeAR

jasnell

@BridgeAR BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Dec 17, 2018

@Trott

@BridgeAR

@nodejs/tsc PTAL. This needs another review due to being semver-major.

@BridgeAR

@BridgeAR

@nodejs/tsc PTAL. This needs some reviews.

@BridgeAR

This changes the loose deep equal comparison by using the same logic as done in the strict deep equal comparison besides comparing primitives loosely and not comparing symbol properties.

assert.deepEqual is still commenly used and this is likely the biggest pitfall.

Most changes are only minor and won't have a big impact besides likely fixing user expectations.

@BridgeAR

@BridgeAR

@nodejs/tsc PTAL.

This PR is open for quite some while and needs some reviews due to being semver-major.

danbev

@mhdawson

Do you have any data for:

I guess this might still be a bit bold but I believe it's the right
thing to do. We just have to check how much impact this has in
userland.

gabrielschulhof

gabrielschulhof

@BridgeAR

ChALkeR

ChALkeR

ChALkeR

@BridgeAR

@BridgeAR

@ChALkeR

Are any of the CITGM failures related to this change?

@BridgeAR

@ChALkeR multiple failures are issues to run the tests on Windows. More failures are related to C++ API removals from V8 and we already track most failures in #25060 and a couple are related to a version. There are a couple further issues besides these that are also unrelated. I could not find any related issue.

@BridgeAR

I'll go ahead and land this if there are no concerns up until the 22.01.

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Jan 27, 2019

@BridgeAR

This changes the loose deep equal comparison by using the same logic as done in the strict deep equal comparison besides comparing primitives loosely, not comparing symbol properties and not comparing the prototype.

assert.deepEqual is still commenly used and this is likely the biggest pitfall.

Most changes are only minor and won't have a big impact besides likely fixing user expectations.

PR-URL: nodejs#25008 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Daniel Bevenius daniel.bevenius@gmail.com

@BridgeAR

Landed in 7493db2 🎉

Thanks everyone for the reviews.

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

@BridgeAR BridgeAR deleted the stricter-loose-comparison branch

January 20, 2020 11:51

@BridgeAR BridgeAR restored the stricter-loose-comparison branch

January 20, 2020 11:52

@BridgeAR BridgeAR deleted the stricter-loose-comparison branch

January 20, 2020 11:53

Reviewers

@ChALkeR ChALkeR ChALkeR left review comments

@gabrielschulhof gabrielschulhof gabrielschulhof left review comments

@danbev danbev danbev approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski Awaiting requested review from apapirovski

@mcollina mcollina Awaiting requested review from mcollina

@targos targos Awaiting requested review from targos

@ofrobots ofrobots Awaiting requested review from ofrobots

Labels

author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

semver-major

PRs that contain breaking changes and should be released in the next major version.

util

Issues and PRs related to the built-in util module.