Skip to content

Conversation

@AkshayIyer12
Copy link
Contributor

Checklist
Affected core subsystem(s)

@nodejs-github-botnodejs-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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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

@mscdexmscdex added the vm Issues and PRs related to the vm subsystem. label May 15, 2017
Copy link
Member

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

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

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

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

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]>
@jasnelljasnell mentioned this pull request May 28, 2017
@gibfahngibfahn 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]>
@MylesBorinsMylesBorins mentioned this pull request Jul 18, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testIssues and PRs related to the tests.vmIssues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@AkshayIyer12@refack@Trott@mscdex@MylesBorins@nodejs-github-bot