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: multiple improvements #21628

Closed
wants to merge 6 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jul 3, 2018

I am going to split this into smaller commits. Therefore it is WIP.

The following changes are in here:

  • Switched + / - | red / green (by request). It does seem to feel a bit more natural this way. Let's hope that most people agree with this.
  • Short primitives do not use the diff anymore (by request). Especially short numbers can be read well like 1 !== 2.
  • Improved error descriptions. It was not really clear how things worked. It should now be clear what everything is about.
  • Added a position indicator for single lines that fit into the tty.
  • Simple assert will now rarely bail out when checking the call site and the output got improved.
  • Some performance optimizations
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

@BridgeAR BridgeAR added wip Issues and PRs that are still a work in progress. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jul 3, 2018
@BridgeAR BridgeAR requested review from devsnek and a team July 3, 2018 00:25
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jul 3, 2018
lib/assert.js Outdated
@@ -128,8 +128,10 @@ function getBuffer(fd, assertLine) {
let startBuffer = 0; // Start reading from that char on
const bytesPerRead = 8192;
const buffers = [];
// Use a single buffer up front that is reused until the call site is found.
let buffer = Buffer.allocUnsafe(bytesPerRead);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

lib/assert.js Outdated
nodes = parseExpressionAt(code, start);
if (nodes.type !== 'CallExpression') {
// Walk the tree to find the call expression.
const walk = require('internal/deps/acorn/dist/walk');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps make this a more "traditional" lazy load by having a top level let walk; then here walk = walk || require('internal/deps/acorn/dist/walk') or something similar?

@devsnek
Copy link
Member

devsnek commented Jul 10, 2018

it might help to just remove the +/- and use colors. i generally try to just focus on the colors because the +/- add another dimension that confuses the heck outta me.

@BridgeAR
Copy link
Member Author

@devsnek using only colors is not an option in case people do not have a TTY with color support. In the end the color is just the same as + / - and only a different notation for the same thing.

@BridgeAR BridgeAR removed the wip Issues and PRs that are still a work in progress. label Jul 13, 2018
@BridgeAR
Copy link
Member Author

I moved the simple assert changes out of this PR. I guess the rest is fine to be in one commit.

So PTAL.

@BridgeAR BridgeAR requested a review from a team July 14, 2018 01:54
@Trott
Copy link
Member

Trott commented Jul 14, 2018

@nodejs/tsc @nodejs/testing

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff changes tested and approved :)

Just a few questions on the messages

strictEqual: 'Input A expected to strictly equal input B',
notStrictEqual: 'Input A expected to strictly not equal input B'
const kReadableOperator = {
deepStrictEqual: 'Expected input to be strictly deep-equal',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: "inputs" instead of "input"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am good either way as it seems that both would work. @Trott @vsemozhetbyt do you have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, "inputs" seems more clear (no confusing "input is deep-equal — to what?"), but I am not a native speaker)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @targos and @vsemozhetbyt for the reason offered by @vsemozhetbyt (using "inputs" removes potential ambiguity).

notStrictEqual: 'Input A expected to strictly not equal input B'
const kReadableOperator = {
deepStrictEqual: 'Expected input to be strictly deep-equal',
notDeepStrictEqual: 'Expected "actual" not to be strictly deep-equal',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use "actual" here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the value of the input argument "actual". I had hoped that this is actually clearer than the version before. Do you have a idea or a suggestion for to make it even better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why it's not input or inputs like with the sibling method

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a combination of this message including the whole other output. Without the further output I agree with you.

If we do a equal comparison the output would look like:

assert.deepStrictEqual([1, 2, 3], [1, 2, 2]);

// Expected inputs to be strictly deep-equal:
// + actual - expected
//
// 
//  [
//    1,
//    2,
// +  3
// -  2
//  ]

While a not equal diff looks like:

assert.notDeepStrictEqual([1, 2, 3], [1, 2, 3]);

// Expected "actual" not be strictly deep-equal
// 
//  [
//    1,
//    2,
//    3
//  ]

So the first part compares two different inputs (actual & expected) while the latter says that "actual" should not be equal to the afterwards printed value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for the explanation, this seems fine.

@BridgeAR
Copy link
Member Author

Comment addressed.

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

@BridgeAR BridgeAR requested a review from a team July 16, 2018 09:30
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit.

(actual !== 0 || expected !== 0)) { // -0 === +0
return `${kReadableOperator[operator]}:\n\n` +
`${actualLines[0]} !== ${expectedLines[0]}\n`;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a comment explaining this block of logic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@BridgeAR
Copy link
Member Author

@BridgeAR BridgeAR requested a review from a team July 16, 2018 15:46
@BridgeAR BridgeAR mentioned this pull request Jul 18, 2018
4 tasks
@BridgeAR BridgeAR requested a review from jasnell July 27, 2018 01:45
// If the stdout is a tty and the input length is lower than the current
// columns per line, add a mismatch indicator below the output.
} else if (process.stdout.isTTY &&
inputLength < process.stdout.columns) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d prefer not to add a dependency on process.stdout here … there’s no guarantee that this is where the error output ends up, and it makes assert depend on something it shouldn’t depend on (or makes it more so, at least).

Can we pick a value, e.g. 80 or 100 or so?

Copy link
Member Author

@BridgeAR BridgeAR Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aware that there is no guarantee that this is where the error output ends up. It is also the reason why I only added the indicator if it is a tty (and yes, even then the error could end up somewhere else but that is unlikely).

If I understand you correct you would actually like to have the indicator all the time? In that case I could add a default of 80 in case it is not a tty and check stdout otherwise?

That all aside: I would actually like to add a proper diffing algorithm at some point with which it would be possible to use colors to highlight the differences instead. But that is a bit more work and I got quite some pushback when it comes to improvements to assert, so I am not really sure if that is something worth trying.

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.
@BridgeAR
Copy link
Member Author

BridgeAR commented Jul 27, 2018

@addaleax I added a default of 80 for all cases where stdout is not a tty.

@targos I also added a "to" to make things even clearer (it addresses #21628 (comment)).

I also moved some code up to remove a intermediate variable and to shorten the code path in that case.

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

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me at a high level.

@BridgeAR
Copy link
Member Author

I just changed two more small things:

  1. Instead of checking stdout, it will now check stderr as it is more likely that the error ends up there.
  2. The indicator is ignored for the first three characters.

@BridgeAR
Copy link
Member Author

@BridgeAR
Copy link
Member Author

I just pushed the doc changes that were missing. The CI was green after resuming as it can be seen by the green check mark next to the commit before the docs. Since the doc changes do not need a full CI, I did not start anything else anymore.

PTAL. I think this is ready otherwise and could land.

@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 2, 2018

PTAL. It would be nice to get LG for the last changes.

@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 3, 2018

@nodejs/documentation PTAL

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc changes LGTM)

@vsemozhetbyt
Copy link
Contributor

Just a nit question. Sometimes in the error logs we have AssertionError [ERR_ASSERTION]: ... and sometimes just AssertionError: .... Is there some system in this?

@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 3, 2018

The line length was to long when adding the ERR_ASSERTION every time. Therefore it was some times removed.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Aug 4, 2018
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>
@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 4, 2018

Thanks a lot.

Landed in 0518b9e 🎉

@BridgeAR BridgeAR closed this Aug 4, 2018
oyyd pushed a commit to oyyd/node that referenced this pull request Sep 25, 2018
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>
@oyyd oyyd mentioned this pull request Sep 26, 2018
2 tasks
@BridgeAR BridgeAR deleted the improve-assert-things branch January 20, 2020 11:34
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. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants