test: fix global.PORT in dgram-cluster-test-1 #12491
Conversation
mscdex
added
cluster
|
Hey Benji, Ok, I found the error log |
|
|
||
| if (common.isWindows) { | ||
| common.skip('dgram clustering is currently not supported ' + | ||
| 'on windows.'); | ||
| 'on windows.'); |
There was a problem hiding this comment.
I'm apologize, It's not related.
There was a problem hiding this comment.
No worries. Thanks for taking the time to do this PR, and I hope the duplicate PR thing won't discourage you from doing more!
There was a problem hiding this comment.
Thanks again, I'm definitely planning to do more..
There was a problem hiding this comment.
הוא מתכוון שכדאי לתקן את זה 😉
אתה יכול לשנות את הקומיט האחרון ועשות פורס-פוש, או להוסיף עוד קומיט.
There was a problem hiding this comment.
@refack please stick to English in comments, thanks :) (so people are not excluded from the discussion). We have the Hebrew locale repo if you'd like to participate in translation and localization efforts.
There was a problem hiding this comment.
Hebrew re: #12491 (comment)
|
It looks like there's another PR that fixes the same issue that came in about 3 hours earlier. #12487 I'd leave this one open until in case that one doesn't land or this one is deemed superior for some reason. As for the CI failure, it looks like a build issue unrelated to this change. I wouldn't worry about it. |
|
ברוך הבא |
|
[No secrets were shared in Hebrew 😉(all Googleable), just me screaching an itch to communicate in the old tongue] |
| s.bind(0, () => { | ||
| const port = s.address().port; | ||
| s.close(); | ||
| cb(null, port); |
There was a problem hiding this comment.
By the time this port is used by the test, there is a possibility that this would be assigned to someother process by the operating system.
There was a problem hiding this comment.
Or Am I missing something here?
There was a problem hiding this comment.
This port assignment was relay on the suggestion of @benjamingr in
#12426 (comment)
Maybe the right solution is to close this port only in the end of the test.
|
Closing as this was already fixed by #12487. Thanks for the contribution though!! |
|
You welcome:) thank you.. |
|
@zivkaziv sorry it "fell between the chairs" we do really appreciate your contribution. 🥇 |
|
Thanks:) I assumed that it wouldn't enter since another PR with this change submitted earlier then me:) but it was a good first commit for me... |
Hi,
This is my first attempt at a pull request here. Attempted to fix the issue referenced below.
Note that we pass the port from the cluster master to the workers via
process.send.Got some (administrative) help from @benjamingr #goodnessSquad
Refs: #12376
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)