FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix test debug agent by larissayvette · Pull Request #9119 · nodejs/node · GitHub

/ node Public

Fix test debug agent - #9119

Closed
larissayvette wants to merge 4 commits into
nodejs:masterfrom
larissayvette:fix-test-debug-agent
ConversationCommits (4)ChecksFiles changed
Closed

Fix test debug agent#9119
larissayvette wants to merge 4 commits into
nodejs:masterfrom
larissayvette:fix-test-debug-agent

Conversation

larissayvette commented Oct 16, 2016

Copy link
Copy Markdown
Contributor
Affected core subsystem(s)

test

Description of change

Cheching if the throwed exception is assert.AssertError

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 16, 2016
cjihrig reviewed Oct 16, 2016
View reviewed changes
Comment thread test/parallel/test-debug-agent.js Outdated
assert.throws(() => { require('_debug_agent').start(); },
assert.AssertionError);
assert.throws(
() => { debug.start();},

cjihrig Oct 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you use consistent spacing around the curly braces.

cjihrig reviewed Oct 16, 2016
View reviewed changes
Comment thread test/parallel/test-debug-agent.js Outdated
assert.AssertionError);
assert.throws(
() => { debug.start();},
function(err) {

cjihrig Oct 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The indentation looks off here.

Trott commented Oct 17, 2016

Copy link
Copy Markdown
Member

LGTM with spacing nits fixed

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

jasnell approved these changes Oct 17, 2016
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

lpinca reviewed Oct 17, 2016
View reviewed changes
Comment thread test/parallel/test-debug-agent.js
() => { debug.start(); },
function(err) {
return (err instanceof assert.AssertionError &&
err.message === 'Debugger agent running without bindings!');

lpinca Oct 17, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Tiny nit: one more space :)

lpinca approved these changes Oct 17, 2016
View reviewed changes

lpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Trott commented Oct 17, 2016

Copy link
Copy Markdown
Member

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

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
jasnell self-assigned this Oct 18, 2016
jasnell pushed a commit that referenced this pull request Oct 18, 2016
test: checking if error constructor is assert.AssertionError
4e1d6d0 
PR-URL: #9119
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

jasnell commented Oct 18, 2016

Copy link
Copy Markdown
Member

Landed in 4e1d6d0

jasnell closed this Oct 18, 2016

jasnell commented Oct 18, 2016

Copy link
Copy Markdown
Member

Fixed the remaining indenting nit on landing.

jasnell pushed a commit that referenced this pull request Oct 18, 2016
test: checking if error constructor is assert.AssertionError
63d8a29 
PR-URL: #9119
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 19, 2016
test: checking if error constructor is assert.AssertionError
8788d00 
PR-URL: #9119
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 15, 2016
test: checking if error constructor is assert.AssertionError
02f7e3a 
PR-URL: #9119
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 15, 2016
test: checking if error constructor is assert.AssertionError
bd99b2d 
PR-URL: #9119
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Nov 22, 2016
Merged
Merged
larissayvette deleted the fix-test-debug-agent branch December 28, 2016 15:51
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig left review comments
jasnell jasnell approved these changes
lpinca lpinca approved these changes

Assignees

jasnell

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL