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.

Sign up for GitHub

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

Jump to bottom

Adding additional error details for asserts. #16116

Closed
wants to merge 1 commit into from
Closed

Adding additional error details for asserts. #16116

wants to merge 1 commit into from

Conversation

burgerboydaddy
Copy link
Contributor

Adding additional error details for asserts. Proper fix for pull request #16009

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 10, 2017
@burgerboydaddy burgerboydaddy mentioned this pull request Oct 10, 2017
Closed
2 tasks
jasnell
jasnell approved these changes Oct 10, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Lgtm.
Just fyi for next time... Opening a new pr is unnecessary but fine. New commits to existing prs can be added. Let one of us know if you need extra assistance figuring that out! Thank you for the contribution!

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Oct 10, 2017
@fhinkel
Copy link
Member

fhinkel commented Oct 12, 2017

Thanks! Can you fix the commit message to adhere to our guidlines? (needs a double line break otherwise it runs on and on, also a subsystem prefix). Much appreciated 😄

@jasnell jasnell added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 16, 2017
@jasnell
Copy link
Member

jasnell commented Oct 16, 2017

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

BridgeAR
BridgeAR reviewed Oct 18, 2017
View reviewed changes
test/parallel/test-vm-context.js Outdated
}, 'Expected appearance of proper offset in Error stack');
assertErrStack = err.stack;
return /expected-filename\.js:33:130/.test(err.stack);
}, `expected appearance of proper offset in Error stack: ${assertErrStack}`);
Copy link
Member

Choose a reason for hiding this comment

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

This test lost two parts that were tested here before. Testing for ^ was definitely intended and should be further tested for. I am also relatively sure the space in front of the throw was intended as well to test for the correct column number.

It would probably be best to just remove the error message overall.

test/parallel/test-vm-context.js Outdated
'expected exception from runInContext signature test');
let eMessage = 'expected exception from ';
eMessage += `runInContext signature test: ${gh1140Exception}`;
assert.ok(gh1140Exception, eMessage);
Copy link
Member

Choose a reason for hiding this comment

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

It would be nicer to just use string concatenation instead of adding a separated variable.

@gireeshpunathil
Copy link
Member

ping @burgerboydaddy

@burgerboydaddy
Copy link
Contributor Author

burgerboydaddy commented Oct 24, 2017 via email

@Trott
Copy link
Member

Trott commented Oct 27, 2017

The cleanup needed here was a little awkward to explain so I just went ahead and did it. Hope that's OK. This should be ready to go, I think . PTAL, especially those of you that approved a previous version. @jasnell @fhinkel @gireeshpunathil

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

@tniessen
Copy link
Member

New linter: https://ci.nodejs.org/job/node-test-linter/13057/

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 29, 2017
@burgerboydaddy @Trott
test: add details in assertions in test-vm-context
5163757 
PR-URL: nodejs#16116
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 29, 2017

Landed in 5163757

@Trott Trott closed this Oct 29, 2017
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@burgerboydaddy @gibfahn
test: add details in assertions in test-vm-context
351a556 
PR-URL: #16116
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@burgerboydaddy @gibfahn
test: add details in assertions in test-vm-context
0e4397c 
PR-URL: #16116
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
@burgerboydaddy @gibfahn
test: add details in assertions in test-vm-context
2e1b45d 
PR-URL: #16116
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Merged
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@burgerboydaddy @Qard
test: add details in assertions in test-vm-context
2bb51ac 
PR-URL: nodejs/node#16116
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@burgerboydaddy @Qard
test: add details in assertions in test-vm-context
e5e844d 
PR-URL: nodejs/node#16116
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
@burgerboydaddy @MylesBorins
test: add details in assertions in test-vm-context
87d80bc 
PR-URL: #16116
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
@burgerboydaddy @MylesBorins
test: add details in assertions in test-vm-context
6ced5d6 
PR-URL: #16116
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
@burgerboydaddy @MylesBorins
test: add details in assertions in test-vm-context
10c24a1 
PR-URL: #16116
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@burgerboydaddy @addaleax
test: add details in assertions in test-vm-context
b698001 
PR-URL: nodejs/node#16116
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@fhinkel fhinkel fhinkel approved these changes

@jasnell jasnell jasnell approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.