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

test: increase _debugger coverage #8403

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Trott
Member

Trott commented Sep 4, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test debugger

Description of change

The uncaught exception test for _debugger.js was not exercising some
code (particularly concerning interface_.child) because of the
synchronous nature of the test. This adds an asynchronous version to
increase test coverage.

test: increase _debugger coverage
The uncaught exception test for `_debugger.js` was not exercising some
code (particularly concerning `interface_.child`) because of the
synchronous nature of the test. This adds an asynchronous version to
increase test coverage.
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 6, 2016

Member

CI is green (except for perma-yellow AIX).

Member

Trott commented Sep 6, 2016

CI is green (except for perma-yellow AIX).

@Trott

This comment has been minimized.

Show comment
Hide comment
Member

Trott commented Sep 6, 2016

assert.doesNotThrow(emit);
debug.start(['sterrance']);

This comment has been minimized.

@indutny

indutny Sep 7, 2016

Member

Wut?

@indutny

This comment has been minimized.

@Trott

Trott Sep 7, 2016

Member

That's sending debug.start() an argv array of length 1 to avoid code that exits if argv is empty.

I'll add a comment and/or change sterrance to a more meaningful string.

@Trott

Trott Sep 7, 2016

Member

That's sending debug.start() an argv array of length 1 to avoid code that exits if argv is empty.

I'll add a comment and/or change sterrance to a more meaningful string.

result.on('close', (code) => {
const expectedMessage =
"There was an internal error in Node's debugger. Please report this bug.";

This comment has been minimized.

@indutny

indutny Sep 7, 2016

Member

Single quotes?

@indutny

indutny Sep 7, 2016

Member

Single quotes?

This comment has been minimized.

@Trott

Trott Sep 7, 2016

Member

I used double-quotes to avoid escaping the apostrophe in Node's. (The linter allows for double-quotes if it avoids escaping a ' character.) I can switch to single-quotes if that's deemed an inadequate reason to use double-quotes.

@Trott

Trott Sep 7, 2016

Member

I used double-quotes to avoid escaping the apostrophe in Node's. (The linter allows for double-quotes if it avoids escaping a ' character.) I can switch to single-quotes if that's deemed an inadequate reason to use double-quotes.

@indutny

This comment has been minimized.

Show comment
Hide comment
@indutny

indutny Sep 7, 2016

Member

LGTM

Member

indutny commented Sep 7, 2016

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 7, 2016

Member

LGTM with nits addressed.

Member

jasnell commented Sep 7, 2016

LGTM with nits addressed.

Trott added a commit to Trott/io.js that referenced this pull request Sep 7, 2016

test: increase _debugger coverage
The uncaught exception test for `_debugger.js` was not exercising some
code (particularly concerning `interface_.child`) because of the
synchronous nature of the test. This adds an asynchronous version to
increase test coverage.

PR-URL: nodejs#8403
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 7, 2016

Member

Landed in 801115d

Member

Trott commented Sep 7, 2016

Landed in 801115d

@Trott Trott closed this Sep 7, 2016

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

test: increase _debugger coverage
The uncaught exception test for `_debugger.js` was not exercising some
code (particularly concerning `interface_.child`) because of the
synchronous nature of the test. This adds an asynchronous version to
increase test coverage.

PR-URL: #8403
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

seeing failures with this being backported

Member

MylesBorins commented Sep 30, 2016

seeing failures with this being backported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment