Uh oh!
There was an error while loading. Please reload this page.
assert: handle undefined filename in getErrMessage - #20848
Closed
jeysal wants to merge 1 commit into
Closed
Conversation
2 tasks
BridgeAR
approved these changes
May 20, 2018
BridgeAR
commented
May 20, 2018
Member
@jeysal thanks a lot for reporting and fixing this! |
BridgeAR
commented
May 20, 2018
Member
trivikr
approved these changes
May 20, 2018
cjihrig
approved these changes
May 20, 2018
addaleax
approved these changes
May 20, 2018
benjamingr
approved these changes
May 20, 2018
benjamingr
left a comment
Member
There was a problem hiding this comment.
Excellent first contribution - good job!
benjamingr
commented
May 20, 2018
Member
CI failures look unrelated. Retried just in case https://ci.nodejs.org/job/node-test-commit/18635/console |
apapirovski
commented
May 21, 2018
Contributor
Just my 2c but wouldn't we prefer to just bail out early before even checking the Map? Then we don't need to store extra values or run all that extra code... |
BridgeAR
commented
May 21, 2018
Member
@apapirovski we could bail out right after checking for a filename but that should not make a significant difference. |
jeysal
commented
May 21, 2018
ContributorAuthor
I guess that would indeed save some cache entries. Might look nicer to have both exit conditions in one place though. Would anyone else prefer splitting them? |
BridgeAR
commented
May 21, 2018
Member
@jeysal if you do not mind, moving would be better. |
jeysalforce-pushed
the
assert-error-message-undefined-filename
branch
from
May 21, 2018 12:30
d267d28 to
86b8330CompareWhen generating an assertion error message, `filename` might be undefined, e.g. if `assert` is called in `eval`. Handle this case gracefully instead of failing with `Cannot read property 'endsWith' of undefined`. Fixes: nodejs#20847
jeysalforce-pushed
the
assert-error-message-undefined-filename
branch
from
May 21, 2018 12:32
86b8330 to
0118fcdComparejeysal
commented
May 21, 2018
ContributorAuthor
done |
XadillaX
approved these changes
May 23, 2018
jasnell
approved these changes
May 23, 2018
jasnell
commented
May 23, 2018
Member
Way too much red and yellow in the last CI run, trying again: https://ci.nodejs.org/job/node-test-pull-request/15055/ |
jasnell
commented
May 23, 2018
Member
Still a lot of red in the CI but none of it related. |
jasnell pushed a commit
that referenced
this pull request
May 23, 2018
When generating an assertion error message, `filename` might be undefined, e.g. if `assert` is called in `eval`. Handle this case gracefully instead of failing with `Cannot read property 'endsWith' of undefined`. Fixes: #20847 PR-URL: #20848 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell
commented
May 23, 2018
Member
Landed in a5d86f8 |
targos pushed a commit
that referenced
this pull request
May 25, 2018
When generating an assertion error message, `filename` might be undefined, e.g. if `assert` is called in `eval`. Handle this case gracefully instead of failing with `Cannot read property 'endsWith' of undefined`. Fixes: #20847 PR-URL: #20848 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Khaidi Chu <i@2333.moe> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When generating an assertion error message,
filenamemight be undefined,e.g. if
assertis called ineval.Handle this case gracefully instead of failing with
Cannot read property 'endsWith' of undefined.Fixes: #20847
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes