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: fix single assert first line #21626

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jul 2, 2018

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 nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Jul 2, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Jul 2, 2018

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
Copy link
Member

Trott commented Jul 3, 2018

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

BridgeAR commented Jul 4, 2018

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
Trott previously approved these changes Jul 4, 2018
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.

Test + fixture LGTM.

@BridgeAR
Copy link
Member Author

BridgeAR commented Jul 4, 2018

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.

@Trott Trott dismissed their stale review July 5, 2018 01:54

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

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.

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

BridgeAR commented Jul 5, 2018

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

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

@BridgeAR
Copy link
Member Author

BridgeAR commented Jul 5, 2018

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

Trott commented Jul 5, 2018

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

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

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?

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 not sure if I understand exactly what you request. The stack trace is not impacted by the code that is read here.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

lib/assert.js Outdated
const buffers = [];
do {
const buffer = Buffer.allocUnsafe(bytesPerRead);
let bytesPerRead = line === 0 ? column + 1000 : 8192;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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.

@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Jul 9, 2018
@BridgeAR 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
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
Copy link
Member Author

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

@BridgeAR 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
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

@mcollina
Copy link
Member

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

@BridgeAR
Copy link
Member Author

All checks passed :)

@targos
Copy link
Member

targos commented Jul 14, 2018

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

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

Landed in 43ee4d6 🎉

@BridgeAR BridgeAR closed this Jul 16, 2018
targos pushed a commit that referenced this pull request Jul 16, 2018
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]>
@targos targos mentioned this pull request Jul 17, 2018
@BridgeAR BridgeAR deleted the fix-assert-first-line 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. 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.

7 participants