lib: make --debug-port work with cluster #306
Conversation
| var port = common.PORT + 42; | ||
| var args = ['--debug-port=' + port, | ||
| common.fixturesDir + '/clustered-server/app.js']; | ||
| var options = {stdio: ['inherit', 'inherit', 'pipe', 'ipc']}; |
There was a problem hiding this comment.
Out of curiosity, what is the reasoning behind this change?
- stdio: [ 'pipe', 'pipe', 'pipe', 'ipc' ]
+ stdio: ['inherit', 'inherit', 'pipe', 'ipc']Also IIRC the convention is to put a space after an opening curly brace and before a closing curling brace.
{ stdio: ['inherit', 'inherit', 'pipe', 'ipc'] }There was a problem hiding this comment.
'inherit' makes console.log() statements visible. Before, stdout output was silently dropped. Makes it harder to debug.
There was a problem hiding this comment.
Apropos the style, a quick grep through the test suite finds more occurrences of [a, b, c] than of [ a, b, c ].
There was a problem hiding this comment.
'inherit' makes console.log() statements visible. Before, stdout output was silently dropped. Makes it harder to debug.
Makes sense, that's a change towards better.
Apropos the style, a quick grep through the test suite finds more occurrences of
[a, b, c]than of[ a, b, c ].
Yes, that's correct. I was pointing out that IIRC { a: 1 } is more common than {a: 1}.
|
@bajtos Any further comments or LGTY? |
|
@bnoordhuis One more comment regarding the coding style, fee free to ignore it. LGTM. |
The test does not work well with concurrent invocations of the test runner because it uses fixed port numbers. The functionality it tests is covered by sequential/test-debug-port-cluster, a verbatim copy with the only difference being that it doesn't use fixed port numbers. PR-URL: nodejs#306 Reviewed-By: Miroslav Bajtoš <[email protected]>
Make the cluster module intercept the `--debug-port=<port>` command line switch and replace it with the debug port of the child process. A happy coincidence of this change is that it finally makes it possible to run the sequential/test-debug-signal-cluster in parallel, it now no longer needs the default port numbers. PR-URL: nodejs#306 Reviewed-By: Miroslav Bajtoš <[email protected]>
Move sequential/test-debug-signal-cluster to test/parallel. Per the previous commit, it can now run in parallel with other debugger tests. PR-URL: nodejs#306 Reviewed-By: Miroslav Bajtoš <[email protected]>
Move sequential/test-debug-port-cluster to test/parallel. This test is safe to run in parallel with other debugger tests, it doesn't use fixed port numbers. PR-URL: nodejs#306 Reviewed-By: Miroslav Bajtoš <[email protected]>
bnoordhuis
force-pushed
the
fix-cluster-debug-port
branch
from
8542ab1
to
c8676cb
Compare
|
Thanks, Miroslav. Sorry, you're right - I misread your style comment. Feedback incorporated and landed in c8676cb. |
Note that parallel/test-debug-port-cluster still fails every now and but at least it fails faster now. It's not a regression.
R=@bajtos?