test changing common.PORT to use 0 within the test-cluster-dgram.reus… #12932
Conversation
mscdex
added
cluster
| @@ -37,4 +37,4 @@ function close() { | |||
| } | |||
|
|
|||
| for (let i = 0; i < 2; i++) | |||
| dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(common.PORT, next); | |||
| dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(0, next); | |||
There was a problem hiding this comment.
I think this would be better if you remove the for loop and have two separate bind() calls. The outer call to bind() would use port 0 and the other call would happen in the callback for that bind(), using .address().port to get the port of the outer bind and reuse it.
There was a problem hiding this comment.
Thanks for your reply!
I made the changes that you requested in your comment above and have implemented them in my latest commit, hopefully correctly.
| for (let i = 0; i < 2; i++) | ||
| dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(0, next); | ||
| const socket = dgram.createSocket({type:'udp4', reuseAddr: true}) | ||
| .bind(0, common.mustCall(()=>{ |
There was a problem hiding this comment.
Commit 7db0f60 Passes lint. Did not realize that my linter was misconfigured in my current workspace. Thanks for this catch!
| dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(common.PORT, next); | ||
| const socket = dgram.createSocket({type: 'udp4', reuseAddr: true}) | ||
| .bind(0, common.mustCall(() => { | ||
| socket.bind(this.address().port, next); |
There was a problem hiding this comment.
This runs bind twice on the socket but the existing test runs createSocket() twice and this runs it only once. But running it twice is what is actually being tested here, I think. So this invalidates the test, I think.
There was a problem hiding this comment.
@Trott Thanks! That makes sense. Would calling another socket inside the callback here work e4f250a ?
| dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(common.PORT, next); | ||
| const socket = dgram.createSocket({type: 'udp4', reuseAddr: true}) | ||
| .bind(0, common.mustCall(() => { | ||
| const socket2 = socket(); |
There was a problem hiding this comment.
socket() looks incorrect to me. Should this be dgram.createSocket()?
There was a problem hiding this comment.
I was under the impression that calling socket() was the same as dgram.createSocket since I declared socket as a const. I updated the code here 41292b7
| const socket = dgram.createSocket({type: 'udp4', reuseAddr: true}) | ||
| .bind(0, common.mustCall(() => { | ||
| const socket2 = dgram.createSocket({type: 'udp4', reuseAddr: true}) | ||
| socket.bind(this.address().port, next); |
There was a problem hiding this comment.
I'm not so happy about this rebind, you could just do next.call(this).
|
IMHO all the literal |
|
P.S. @akopcz2 thank you very much for your contribution 🥇 |
|
@refack Absolutely. Thanks for the explanation! |
|
@Trott No worries...and absolutely. Didn't realize contributing would be this fun. Hopefully next time I can get a change in to master 💯 |
…e.js
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)