Skip to content

Conversation

AkshayIyer12
Copy link
Contributor

Checklist
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 15, 2017
Copy link
Member

Choose a reason for hiding this comment

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

Is this addition a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this added line. Thanks

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label May 15, 2017
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.

Please remove the added line. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Please remove this added line. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

@refack
Copy link
Contributor

refack 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

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.

LGTM

@Trott
Copy link
Member

Trott commented May 19, 2017

Landed in a593c74.
Thanks for the contribution! 🎉

Trott pushed a commit that referenced this pull request May 19, 2017
Changed the second parameter of assert.throws to match the errors.

PR-URL: #13035
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Changed the second parameter of assert.throws to match the errors.

PR-URL: nodejs#13035
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
MylesBorins pushed a commit that referenced this pull request Jul 17, 2017
Changed the second parameter of assert.throws to match the errors.

PR-URL: #13035
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants