assert: fix single assert first line #21626

BridgeAR · 2018-07-02T20:57:16Z

If simple assert is called in the very first line of a file and it
causes an error, it will report the wrong code. The reason is that
the column that is reported is faulty.

I tested this locally as it is not as nice to write a proper test for this and I am uncertain if it is worth it adding one. I feel it should be fine to land this without a test if no one disagrees.

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

nodejs-github-bot · 2018-07-02T20:57:17Z

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/199/pipeline

BridgeAR · 2018-07-02T21:32:15Z

Just so people know in what way this could fail:

'use strict'; const assert = require('assert'); assert('');

// aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(false);

This fails with: assert(false) and it should be assert('') instead.

Trott · 2018-07-03T04:35:29Z

I won't push hard for it, but my thinking is that you might as well write a test because if you don't, that line will show up as uncovered in our coverage report and someone will write a test for it but they may not know what to test for. Since you have all the context, might as well write the test. Sets a good example for others too, I suppose.

Easiest way to make it work might be to put your test code above in a fixture and invoke the fixture using the child_process module. Fixtures don't get linted so there won't be any lint problems to overcome.

BridgeAR · 2018-07-04T07:29:47Z

There is actually a simple solution to properly fix the issue, so I went for that and added a test case as well.

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

PTAL

Trott

Test + fixture LGTM.

BridgeAR · 2018-07-04T23:27:33Z

I pushed another commit that should be squashed later on.

The former algorithm did not work for the first line and it was not working well with very long lines (this could be the case for e.g. minified code. This addresses that by now only getting exactly what is necessary to produce the correct output. This reduces the number of reads and allocations in multiple cases.

I looked at the test and fixture, but the actual code change needs careful review from someone...

mcollina

TEST+Fixture LGTM.

Can you add some more comments in the algorithm? it's not really readable atm. A description on what's getBuffer does would also be very helpful.

BridgeAR · 2018-07-05T15:52:30Z

I just added a couple of comments to clarify the algorithm. I hope that makes it clearer what is actually going on.

mcollina

LGTM

BridgeAR · 2018-07-05T15:59:19Z

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

Trott · 2018-07-05T18:07:50Z

Would be good to get another review on this from someone. @nodejs/testing

bcoe

just a couple questions for you -- if it is possible to check line # in the assertion this would be nice, but sounds like it's a hassle?

bcoe · 2018-07-05T18:14:27Z

test/parallel/test-assert-first-line.js

+  () => require(path('assert-first-line')),
+  {
+    name: 'AssertionError [ERR_ASSERTION]',
+    message: "The expression evaluated to a falsy value:\n\n  assert.ok('')\n"


could we assert that line 1 shows up somewhere in the stack trace, I imagine you already thought of this and it's too much of a hassle?

I am not sure if I understand exactly what you request. The stack trace is not impacted by the code that is read here.

Since this bug was only manifesting itself if an exception happened on the first line of the application, I was wondering if it's worthwhile asserting that this was the line in the stack trace that caused the error.

This is probably overkill, since the fixture name assert-first-line is pretty clear about what's being tested. No blockers from me.

Since the fixture itself can only throw on line 1, I think it is fine to keep it as is :)

bcoe · 2018-07-05T18:15:06Z

lib/assert.js

-  const buffers = [];
-  do {
-    const buffer = Buffer.allocUnsafe(bytesPerRead);
+  let bytesPerRead = line === 0 ? column + 1000 : 8192;


purely out of curiosity, why switch to reading 1000 bytes at a time rather than 8192? I'm guessing both values are somewhat arbitrary?

It does not read 1000 bytes at a time but the exact number of bytes necessary. So we either safe reading to many bytes (the necessary bytes could be smaller than 8kb) or we safe extra read calls (we know that the necessary bytes are more than 8kb). So it just optimizes for these two cases and in all other cases it will still read exactly 8kb on each read.

1) If simple assert is called in the very first line of a file and it causes an error, it used to report the wrong code. The reason is that the column that is reported is faulty. This is fixed by subtracting the offset from now on in such cases. 2) The actual code read is now limited to the part that is actually required to visualize the call site. All other code in e.g. minified files will not cause a significant overhead anymore. 3) The number of allocations is now significantly lower than it used to be. The buffer is reused until the correct line in the code is found. In general the algorithm tries to safe operations where possible. 4) The indentation is now corrected depending on where the statement actually beginns. 5) It is now possible to handle `.call()` and `.apply()` properly. 6) The user defined function name will now always be used instead of only choosing either `assert.ok()` or `assert()`.

BridgeAR · 2018-07-13T16:25:46Z

@bcoe @mcollina @Trott PTAL. I had to do further adjustments as my former algorithm struggled with multi byte characters. So took a step back and rewrote the algorithm from scratch. All the changes are listed in the commit message.

Now it contains more changes than just fixing the first line as it also improves the output, the indentation, the performance and it is able to handle more input properly. In the end I still believe that this is a patch though as it just fixes things in a way as they should have been before. It will also not impact most use cases.

BridgeAR · 2018-07-13T16:27:25Z

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

mcollina

LGTM

mcollina · 2018-07-14T09:02:19Z

It seems some CI is failing, can you click “resume build”
to check if it passes?

BridgeAR · 2018-07-14T13:58:26Z

All checks passed :)

targos · 2018-07-14T14:01:14Z

Passing CI: https://ci.nodejs.org/job/node-test-commit/19780/

mcollina

LGTM

1) If simple assert is called in the very first line of a file and it causes an error, it used to report the wrong code. The reason is that the column that is reported is faulty. This is fixed by subtracting the offset from now on in such cases. 2) The actual code read is now limited to the part that is actually required to visualize the call site. All other code in e.g. minified files will not cause a significant overhead anymore. 3) The number of allocations is now significantly lower than it used to be. The buffer is reused until the correct line in the code is found. In general the algorithm tries to safe operations where possible. 4) The indentation is now corrected depending on where the statement actually beginns. 5) It is now possible to handle `.call()` and `.apply()` properly. 6) The user defined function name will now always be used instead of only choosing either `assert.ok()` or `assert()`. PR-URL: nodejs#21626 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: James M Snell <[email protected]>

BridgeAR · 2018-07-16T09:33:36Z

Landed in 43ee4d6 🎉

1) If simple assert is called in the very first line of a file and it causes an error, it used to report the wrong code. The reason is that the column that is reported is faulty. This is fixed by subtracting the offset from now on in such cases. 2) The actual code read is now limited to the part that is actually required to visualize the call site. All other code in e.g. minified files will not cause a significant overhead anymore. 3) The number of allocations is now significantly lower than it used to be. The buffer is reused until the correct line in the code is found. In general the algorithm tries to safe operations where possible. 4) The indentation is now corrected depending on where the statement actually beginns. 5) It is now possible to handle `.call()` and `.apply()` properly. 6) The user defined function name will now always be used instead of only choosing either `assert.ok()` or `assert()`. PR-URL: #21626 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jul 2, 2018

BridgeAR force-pushed the fix-assert-first-line branch from 3e2b925 to 83e32f6 Compare July 4, 2018 07:27

Trott previously approved these changes Jul 4, 2018

View reviewed changes

BridgeAR force-pushed the fix-assert-first-line branch from 0a721e4 to 93a1fbf Compare July 4, 2018 23:23

mcollina reviewed Jul 5, 2018

View reviewed changes

mcollina approved these changes Jul 5, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 5, 2018

bcoe approved these changes Jul 5, 2018

View reviewed changes

BridgeAR added the wip Issues and PRs that are still a work in progress. label Jul 9, 2018

BridgeAR force-pushed the fix-assert-first-line branch from 536a0e7 to d1638f4 Compare July 13, 2018 16:11

BridgeAR removed wip Issues and PRs that are still a work in progress. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 13, 2018

BridgeAR force-pushed the fix-assert-first-line branch from d1638f4 to b8fb486 Compare July 13, 2018 16:18

jasnell approved these changes Jul 13, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 14, 2018

mcollina approved these changes Jul 14, 2018

View reviewed changes

mcollina approved these changes Jul 15, 2018

View reviewed changes

BridgeAR closed this Jul 16, 2018

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

BridgeAR deleted the fix-assert-first-line branch January 20, 2020 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: fix single assert first line #21626

assert: fix single assert first line #21626

BridgeAR commented Jul 2, 2018

nodejs-github-bot commented Jul 2, 2018

BridgeAR commented Jul 2, 2018

Trott commented Jul 3, 2018

BridgeAR commented Jul 4, 2018

Trott left a comment

BridgeAR commented Jul 4, 2018

mcollina left a comment

BridgeAR commented Jul 5, 2018

mcollina left a comment

BridgeAR commented Jul 5, 2018

Trott commented Jul 5, 2018

bcoe left a comment

bcoe Jul 5, 2018

BridgeAR Jul 6, 2018

bcoe Jul 6, 2018

BridgeAR Jul 9, 2018

bcoe Jul 5, 2018

BridgeAR Jul 6, 2018

BridgeAR commented Jul 13, 2018

BridgeAR commented Jul 13, 2018

mcollina left a comment

mcollina commented Jul 14, 2018

BridgeAR commented Jul 14, 2018

targos commented Jul 14, 2018

mcollina left a comment

BridgeAR commented Jul 16, 2018

assert: fix single assert first line #21626

assert: fix single assert first line #21626

Conversation

BridgeAR commented Jul 2, 2018

Checklist

nodejs-github-bot commented Jul 2, 2018

BridgeAR commented Jul 2, 2018

Trott commented Jul 3, 2018

BridgeAR commented Jul 4, 2018

Trott left a comment

Choose a reason for hiding this comment

BridgeAR commented Jul 4, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Jul 5, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Jul 5, 2018

Trott commented Jul 5, 2018

bcoe left a comment

Choose a reason for hiding this comment

bcoe Jul 5, 2018

Choose a reason for hiding this comment

BridgeAR Jul 6, 2018

Choose a reason for hiding this comment

bcoe Jul 6, 2018

Choose a reason for hiding this comment

BridgeAR Jul 9, 2018

Choose a reason for hiding this comment

bcoe Jul 5, 2018

Choose a reason for hiding this comment

BridgeAR Jul 6, 2018

Choose a reason for hiding this comment

BridgeAR commented Jul 13, 2018

BridgeAR commented Jul 13, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jul 14, 2018

BridgeAR commented Jul 14, 2018

targos commented Jul 14, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Jul 16, 2018