-
Notifications
You must be signed in to change notification settings - Fork 30k
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: move test-net-connect-handle-econnrefused to sequential #27014
Conversation
The port used in the test could be taken by another process before the callback of `server.close()` is called. Move it to sequential. Refs: nodejs#18257 (comment) Fixes: nodejs#26907
- Remove unneeded server - Use `common.PORT`
I don't see how this really changes anything? |
@mscdex it is no longer run in parallel. First commit moves it to test/sequential. The assumption is that |
Instead of moving this test, why don't we just try to connect to port 0 on localhost? That should always fail with |
Sounds good, will update in a bit. |
@mscdex it doesn't work on Windows. A different errno/code is returned. |
FWIW it looks like we could do this, but it's a little more involved for Windows. Instead of simply connecting to port 0 (which I think we could safely do for non-Windows) you can create a child process running the TCP server on a random port, suspend the process (doable via powershell), and then make connections to the server until you get |
What's the status here? |
Waiting for feedbacks/reviews. In my opinion @mscdex's latest suggestion is too complex to solve an issue with a test that has problem when run in parallel. We already have a dedicated set of tests running sequentially. cc: @nodejs/testing |
@nodejs/collaborators any more opinions/suggestions? |
The port used in the test could be taken by another process before the callback of `server.close()` is called. Move it to sequential. PR-URL: nodejs#27014 Fixes: nodejs#26907 Refs: nodejs#18257 (comment) Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
- Remove unneeded server - Use `common.PORT` PR-URL: nodejs#27014 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Landed in eca71e5...66cf4b5. |
The port used in the test could be taken by another process before the callback of `server.close()` is called. Move it to sequential. PR-URL: #27014 Fixes: #26907 Refs: #18257 (comment) Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
- Remove unneeded server - Use `common.PORT` PR-URL: #27014 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
First commit:
Second commit:
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes