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

Changed the second parameter of assert.throws to match the errors #13035

Closed
wants to merge 0 commits into from

Conversation

@AkshayIyer12
Copy link
Contributor

commented May 15, 2017

Checklist
Affected core subsystem(s)
test/parallel/test-vm-new-script-new-context.js Outdated


console.error('undefined reference');
script = new Script('foo.bar = 5;');
assert.throws(function() {
script.create;

This comment has been minimized.

Copy link
@Trott

Trott May 15, 2017

Member

Is this addition a mistake?

This comment has been minimized.

Copy link
@AkshayIyer12

AkshayIyer12 May 15, 2017

Author Contributor

No it was there earlier, but it had no semi-colon. So it was one of the errors that popped up during make jslint

This comment has been minimized.

Copy link
@AkshayIyer12

AkshayIyer12 May 15, 2017

Author Contributor

screenshot from 2017-05-14 22-09-59

This comment has been minimized.

Copy link
@AkshayIyer12

This comment has been minimized.

Copy link
@Trott

Trott May 16, 2017

Member

Please remove this added line. Thanks

@mscdex mscdex added the vm label May 15, 2017

@Trott
Copy link
Member

left a comment

Please remove the added line. Thanks.

test/parallel/test-vm-new-script-new-context.js Outdated


console.error('undefined reference');
script = new Script('foo.bar = 5;');
assert.throws(function() {
script.create;

This comment has been minimized.

Copy link
@Trott

Trott May 16, 2017

Member

Please remove this added line. Thanks

test/parallel/test-vm-new-script-new-context.js Outdated


console.error('undefined reference');
script = new Script('foo.bar = 5;');
assert.throws(function() {

This comment has been minimized.

Copy link
@Trott

Trott May 16, 2017

Member

Still adding a blank line. Can we remove that addition, please?

@refack
refack approved these changes May 17, 2017
Copy link
Member

left a comment

LGTM pending CI

@refack

This comment has been minimized.

Copy link
Member

commented May 17, 2017

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

P.S. There might be a problem to land this, @AkshayIyer12 we'll need you to "rebase" this PR

@Trott
Trott approved these changes May 18, 2017
Copy link
Member

left a comment

LGTM

@Trott Trott closed this May 18, 2017

@Trott Trott force-pushed the AkshayIyer12:master branch to 29d89c9 May 18, 2017

@Trott

This comment has been minimized.

Copy link
Member

commented May 19, 2017

Landed in a593c74.
Thanks for the contribution! 🎉

Trott added a commit that referenced this pull request May 19, 2017
test: refactor test-vm-new-script-new-context
Changed the second parameter of assert.throws to match the errors.

PR-URL: #13035
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: refactor test-vm-new-script-new-context
Changed the second parameter of assert.throws to match the errors.

PR-URL: nodejs#13035
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
MylesBorins added a commit that referenced this pull request Jul 17, 2017
test: refactor test-vm-new-script-new-context
Changed the second parameter of assert.throws to match the errors.

PR-URL: #13035
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.