Skip to content
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: enable test-debugger-pid #12770

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 30, 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.

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

test debugger inspector

@Trott Trott 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
Member Author

Trott commented Apr 30, 2017

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

@Trott
Copy link
Member Author

Trott commented Apr 30, 2017

@Trott
Copy link
Member Author

Trott commented Apr 30, 2017

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
Member Author

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...

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

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

addaleax commented May 3, 2017

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
Member Author

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 <jan.krems@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@Trott
Copy link
Member Author

Trott commented May 4, 2017

Landed in 4677766

@Trott Trott 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 <jan.krems@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@Trott Trott deleted the debugger-test branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants