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: inspect getters #25004

Closed

Conversation

BridgeAR
Copy link
Member

While asserting two objects the descriptor is not taken into account.
Therefore getters will be triggered as such. This makes sure they
are also highlighted in the error message instead of potentially
looking identical while the return value of the getter is actually
different.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

While asserting two objects the descriptor is not taken into account.
Therefore getters will be triggered as such. This makes sure they
are also highlighted in the error message instead of potentially
looking identical while the return value of the getter is actually
different.
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Dec 13, 2018
@BridgeAR
Copy link
Member Author

@BridgeAR
Copy link
Member Author

@nodejs/assert PTAL

@Trott
Copy link
Member

Trott commented Dec 14, 2018

This could conceivably make tests fail where they currently succeed. In many cases, that's probably more of a bugfix than a breaking change. But not all, I don't think.

This is the internal conversation I'm having with myself that has prevented me from approving this up until now. It looks good to me, but I wonder.

Is a CITGM run a good idea here?

@BridgeAR
Copy link
Member Author

@Trott I am sorry but I am not sure I can follow. Do you mean a test that checks the assertion message? We changed that multiple times so far and I am not aware of any issues about that (anymore).

Just so we are aligned: the actual comparison is not changed here at all. That comparison already triggers the getters and it's just the error output which is changed.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 16, 2018
@BridgeAR
Copy link
Member Author

@Trott
Copy link
Member

Trott commented Dec 17, 2018

Resume Build had 100% failure on fanned jobs, which I'm guessing is because something cached got dropped from Jenkins. CI: https://ci.nodejs.org/job/node-test-pull-request/19634/ (scheduled)

@BridgeAR
Copy link
Member Author

Landed in 3b2698e

@BridgeAR BridgeAR closed this Dec 18, 2018
BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 18, 2018
While asserting two objects the descriptor is not taken into account.
Therefore getters will be triggered as such. This makes sure they
are also highlighted in the error message instead of potentially
looking identical while the return value of the getter is actually
different.

PR-URL: nodejs#25004
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
While asserting two objects the descriptor is not taken into account.
Therefore getters will be triggered as such. This makes sure they
are also highlighted in the error message instead of potentially
looking identical while the return value of the getter is actually
different.

PR-URL: #25004
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 25, 2018
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
While asserting two objects the descriptor is not taken into account.
Therefore getters will be triggered as such. This makes sure they
are also highlighted in the error message instead of potentially
looking identical while the return value of the getter is actually
different.

PR-URL: nodejs#25004
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR BridgeAR deleted the add-getters-to-assertion-errors branch January 20, 2020 11:48
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants