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

[v10.x] Backport assert PRs #23095

Closed
wants to merge 3 commits into from
Closed

Conversation

oyyd
Copy link
Contributor

@oyyd oyyd commented Sep 26, 2018

Backport of #21628, #22155, #22788. Closes #22711.

Only modify one line code of tests to match #20857.

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

1) Switched + / - and red / green in diffs. It seems like that style
   is more natural to most people.

2) Short primitives do not use the diff anymore. Especially short
   numbers can be read well like 1 !== 2. Cases that can not be
   displayed like that (e.g., -0 and +0) use the regular diff output.

3) Improved error descriptions. It was not always clear what the
   messages stood for. That should now be resolved.

4) Added a position indicator for single lines in case a tty is used
   and the line is shorter than the visual columns.

5) Color detection is now done by checking stderr instead of stdout.

PR-URL: nodejs#21628
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
So far the error message from a loose assertion is not always very
informative and could be misleading. This is fixed by:

* showing more from the actual error message
* having a better error description
* not using custom inspection
* inspecting a higher depth
* inspecting the full array instead of up to 100 entries

PR-URL: nodejs#22155
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

PR-URL: nodejs#22788
Refs: nodejs#22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. v10.x labels Sep 26, 2018
@oyyd
Copy link
Contributor Author

oyyd commented Sep 26, 2018

@BridgeAR @targos PTAL.

@BridgeAR
Copy link
Member

Hm, the first commit (#21628) is semver-major. It should not be backported. We might decide otherwise if the TSC agrees to that (pretty much any assert PR will have to be backported manually if that PR is not backported as well, since it changes the actual / expected order).

@BridgeAR
Copy link
Member

What do others think about backporting it to 10?

@oyyd
Copy link
Contributor Author

oyyd commented Sep 26, 2018

I haven't noticed that it's semver-major and that commit is a big change, I think. I would close this PR.

@BridgeAR I'm sorry to be such a bother.

@oyyd oyyd closed this Sep 26, 2018
@BridgeAR
Copy link
Member

@oyyd no, we have to thank you for opening the PR and investing your time into this! :-)

@oyyd
Copy link
Contributor Author

oyyd commented Sep 27, 2018

Thanks :)

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants