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: add inspect-brk option to cluster module #12503

Closed
wants to merge 4 commits into from

Conversation

dave-k
Copy link
Contributor

@dave-k dave-k commented Apr 19, 2017

Ensure that cluster interoperates with the --inspect-brk option.
This does not test for —debug-brk.

Fixes: #11420

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

test cluster

Ensure that cluster interoperates with the --inspect-brk option.
This does not test for —debug-brk.

Fixes: nodejs#11420
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 19, 2017
@hiroppy hiroppy added the cluster Issues and PRs related to the cluster subsystem. label Apr 19, 2017
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Slightly confusing way to write this - but still LGTM.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 19, 2017

It might be helpful to have the cluster worker print something or crash. Then, you can test that it doesn't happen because the worker is at a breakpoint.

Have the cluster worker print something or crash.
Then, test that it doesn't happen
because the worker is at a breakpoint.
test([`--inspect-brk=${debuggerPort}`]);
} else {
// Cluster worker is at a breakpoint, should not reach here.
assert.fail(1, 2, 'Test failed: cluster worker is at a breakpoint.', '>');
Copy link
Contributor

Choose a reason for hiding this comment

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

As of recently, this can just be:

assert.fail('Test failed: cluster worker is at a breakpoint.');

@dave-k
Copy link
Contributor Author

dave-k commented Apr 20, 2017

Are the requested changes OK?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM if the CI is good. However, because this test uses common.PORT, it should probably be moved from parallel to sequential.

@dave-k
Copy link
Contributor Author

dave-k commented Apr 24, 2017

Can this proceed or does the test need to be moved from parallel to sequential?

@cjihrig
Copy link
Contributor

cjihrig commented Apr 25, 2017

I would move it to sequential as long as common.PORT is there.

because this test uses common.PORT, moved from parallel to sequential.
@cjihrig
Copy link
Contributor

cjihrig commented Apr 25, 2017

@addaleax
Copy link
Member

Landed in 0324ac6

@addaleax addaleax closed this Apr 29, 2017
addaleax pushed a commit that referenced this pull request Apr 29, 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>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

Should land with #12615 if that lands

MylesBorins pushed a commit that referenced this pull request 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 MylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request 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>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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