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

Implement a test to ensure that cluster properly interoperates with the --{inspect,debug}-brk options #11420

Closed
ofrobots opened this issue Feb 16, 2017 · 12 comments
Labels
good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@ofrobots
Copy link
Contributor

ofrobots commented Feb 16, 2017

When started with any of the --inspect* or --debug* options, the cluster module is supposed to provide a unique port to the child processes. The existing test for this functionality seems to lack ability to test the --inspect-brk and --debug-brk flags.

It would be good to add a test (or modify the existing test) to be add this capability.

See: #11386 (comment).

@ofrobots ofrobots added good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Feb 16, 2017
@lvpx
Copy link

lvpx commented Feb 21, 2017

Hi, I would like to give it a try. I have never contributed to any project before.

@gibfahn
Copy link
Member

gibfahn commented Feb 22, 2017

@lovepreetkaul Great, go for it!

Contributing guidelines are here, if you follow them carefully you shouldn't have a problem with the GitHub side of things. If you need any help then feel free to comment on here.

@ofrobots
Copy link
Contributor Author

I'd be happy to help answer any questions. My suggestion would be to look into understanding how the existing test that verifies that --inspect and --debug work with cluster.

The complication that --debug-brk and --inspect-brk cause is that they would cause the child process to stop immediately. The test would have to anticipate this and somehow cause the child to continue. One suggestion on how to do that would be to look at how some tests that use --debug-brk work (e.g. test-debug-brk.js, etc.)

@dave-k
Copy link
Contributor

dave-k commented Apr 1, 2017

@lovepreetkaul are working this issue?

I would like to give it a try if no-one is working on this.

@lvpx
Copy link

lvpx commented Apr 1, 2017

@dave-k yes please go ahead.

@dave-k
Copy link
Contributor

dave-k commented Apr 4, 2017

@ofrobots

I have read and understand how the existing tests work, but need some help with your suggestion.
How do you propose that the --debug-brk test cause the child to continue ?

@ofrobots
Copy link
Contributor Author

ofrobots commented Apr 5, 2017

@dave-k Thanks for looking into this. I suspect that it might not be necessary to cause the child to continue for the purposes of this test. I think what we need to ensure is that the child started on the correct port – which we can detect based on the stdout from the child. Once you have received this output, I think you can terminate the child process (i.e. something similar to test-debug-brk.js).

@dave-k
Copy link
Contributor

dave-k commented Apr 10, 2017

@ofrobots

I have a test for the --inspect-brk option working, but
the --debug-brk option is not supported in node v8.0.0-pre.
Should I commit the --inspect-brk test separately?

$ ./node --debug-brk
./node: bad option: --debug-brk
$ ./node --version
v8.0.0-pre

@dave-k
Copy link
Contributor

dave-k commented Apr 18, 2017

What are the next steps with this issue?
Should I commit the --inspect-brk test separately?

@ofrobots
Copy link
Contributor Author

Hi @dave-k. I missed your last message, sorry. A test just for clustered --inspect-brk sounds fine to me. Do you want to open a PR with it?

@gibfahn
Copy link
Member

gibfahn commented Apr 18, 2017

I think the outcome of #12364 was that we should probably re-add --debug-brk to master, so a test for that would still be useful, but agreed that it can be in a separate PR.

@dave-k
Copy link
Contributor

dave-k commented Apr 18, 2017

OK I'll remove the test for --debug-brk and open a PR for --inspect-brk

dave-k added a commit to dave-k/node that referenced this issue Apr 19, 2017
Ensure that cluster interoperates with the --inspect-brk option.
This does not test for —debug-brk.

Fixes: nodejs#11420
MylesBorins pushed a commit that referenced this issue Oct 16, 2017
Ensure that cluster interoperates with the --inspect-brk option.
This does not test for --debug-brk.

Fixes: #11420
PR-URL: #12503
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 25, 2017
Ensure that cluster interoperates with the --inspect-brk option.
This does not test for --debug-brk.

Fixes: #11420
PR-URL: #12503
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants