test: fix parallel/test-debug-signal-cluster #501
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bnoordhuis
changed the title
| @@ -4,22 +4,21 @@ var spawn = require('child_process').spawn; | |||
|
|
|||
| var debugPort = common.PORT; | |||
| var args = ['--debug-port=' + debugPort]; | |||
There was a problem hiding this comment.
shouldn't this be in sequential if it's doing a listen?
There was a problem hiding this comment.
No, there are plenty of parallel tests that listen on a port. The logic for that is in tools/test.py and test/common.js: the former sets TEST_THREAD_ID which the latter turns into a per-thread port range.
Aside: 'thread' is a bit of a misnomer because they are really separate processes.
|
I'm not really grokking the changes in test-debug-signal-cluster.js with a quick scan but it's passing CI so 👍 |
|
lgtm |
Make this test less prone to race conditions by using synchronous interprocess communication instead of a timer to determine when the child process is ready to receive messages from its parent. Also, remove a superfluous timer since the tests suite already makes tests time out after a while. Original-PR-URL: nodejs/node-v0.x-archive#9049 Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com> PR-URL: nodejs#501 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Move sequential/test-debug-port-from-cmdline to test/parallel. Per the previous commit, it should now be possible to run the test in parallel with other debugger tests. PR-URL: nodejs#501 Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Temporarily revert the changes to test/ from commit 11c1bae ("lib: make --debug-port work with cluster") to ease cherry-picks from joyent/node. PR-URL: nodejs#501 Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
unref one superfluous timer (as the test suite already has a global timeout), and improve the state machine to iterate the messages more reliably. Ultimately make the test complete more quickly. Original-PR-URL: [unknown] Signed-off-by: Julien Gilli <julien.gilli@joyent.com> PR-URL: nodejs#501 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Reland the changes from commit 11c1bae ("lib: make --debug-port work with cluster") that were temporarily backed out to cherry-pick commits from joyent/node. PR-URL: nodejs#501 Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Previously if a worker's state machine had already transitioned into the 'listening' state when it received the message enabling the debugger, the worker would never enable its debugger. Change the logic to allow the 'listening' as a valid state for enabling the debugger. Fixes: nodejs/node-v0.x-archive#6440 Original-PR-URL: nodejs/node-v0.x-archive#9037 Signed-off-by: Julien Gilli <julien.gilli@joyent.com> Fixes: nodejs#340 PR-URL: nodejs#501 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
bnoordhuis
force-pushed
the
fix-issue-340
branch
from
1d3d3ca
to
4dd22b9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #340
R=@piscisaureus and @rvagg
I cherry-picked some of the commits from joyent/node@v0.12. The commit logs don't mention the original SHA but I did try to link to the relevant PR where possible. Suggestions for improvement welcome.
https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/93/