Skip to content

Conversation

@Trott
Copy link
Member

Now that node debug is an alias for node inspect, it's possible that
node-debug-pid can run reliably. Modify for current behavior and move
from disabled to parallel.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test debugger inspector

@TrottTrott added debugger wip Issues and PRs that are still a work in progress. inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. labels Apr 30, 2017
@Trott
Copy link
MemberAuthor

Labeled as in progress pending CI results as I'm not able to run on Windows etc. locally.

@Trott
Copy link
MemberAuthor

@Trott
Copy link
MemberAuthor

Hmmm...Windows fails. A few options:

  • skip the test on Windows
  • bifurcated logic to handle Windows behavior
  • remove the test entirely as it's mostly testing the behavior of the node-inspect dependency anyway and not Node.js core itself

Thoughts? /cc @jkrems

@Trott
Copy link
MemberAuthor

Trott commented May 1, 2017

Added code to skip parts of the test on Windows.

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

Because I hope/expect this to pass CI, I'm going to remove the in progress label too...

@TrottTrott removed the wip Issues and PRs that are still a work in progress. label May 1, 2017
Copy link
Contributor

@hybristhybrist left a comment

Choose a reason for hiding this comment

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

I think in the future we should investigate adding make test-node-inspect to our general CI runs. Until then having some smoke tests like this one as part of node's own test suite is definitely valuable.

@addaleax
Copy link
Member

Linter failure:

Running JS linter... ./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.md \ benchmark doc lib test tools /home/xxxx/src/node/master/test/parallel/test-debugger-pid.js 13:7 error Expected a function declaration func-style ✖ 1 problem (1 error, 0 warnings) 

Now that `node debug` is an alias for `node inspect`, it's possible that `node-debug-pid` can run reliably. Modify for current behavior and move from `disabled` to `parallel`.
@Trott
Copy link
MemberAuthor

Trott commented May 4, 2017

Rebased and adjusted to conform with new lint rule.

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

Trott added a commit to Trott/io.js that referenced this pull request May 4, 2017
Now that `node debug` is an alias for `node inspect`, it's possible that `node-debug-pid` can run reliably. Modify for current behavior and move from `disabled` to `parallel`. PR-URL: nodejs#12770 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
@Trott
Copy link
MemberAuthor

Trott commented May 4, 2017

Landed in 4677766

@TrottTrott closed this May 4, 2017
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
Now that `node debug` is an alias for `node inspect`, it's possible that `node-debug-pid` can run reliably. Modify for current behavior and move from `disabled` to `parallel`. PR-URL: nodejs#12770 Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
@jasnelljasnell mentioned this pull request May 11, 2017
@gibfahngibfahn mentioned this pull request Jun 15, 2017
3 tasks
@TrottTrott deleted the debugger-test branch January 13, 2022 22:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inspectorIssues and PRs related to the V8 inspector protocoltestIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@Trott@addaleax@refack@jasnell@hybrist@cjihrig@gibfahn