-
Notifications
You must be signed in to change notification settings - Fork 654
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
Ensure all tests use unique IP ports #2196
Conversation
There still seems to be tests that are using the same port 8080 in Lines 2560 to 2597 in dbdcfed
BTW, If we want to maintain this state, what about defining some helper type like: /// Helper type for ensuring all ports used in tests are unique
struct PortAllocator {
next: AtomicU16,
}
impl PortAllocator {
pub fn allocate_port(&self) -> u16 {
self.next.fetch_add(1, Ordering::Relaxed)
}
} Though it may be easier to clash (with the ports used on developer's host) comparing to manually-specified ports |
In the case of the 8080 ports, you'll notice that none of those tests actually bind to the ports in question. So there's no real conflict. Regarding the allocator, I've wondered about that too. But I wonder if the addition of the allocator would obscure the function of some of the tests. Do you think so? |
Yeah, that's right, they are not in actual use. I saw the changes in
Emmm, I am not sure about this:D, would you like show me a brief example demostrating your worries? |
I'm just thinking about readability for somebody not highly familiar with this project. Code like this: let addr = SockaddrIn::from_str("127.0.0.1:1234"); is a lot more obvious than this: let addr = crate::port_allocator::next(); |
…rolMessageOwned (#2187) * Added FreeBSD's SCM_REALTIME and SCM_MONOTONIC into sys::socket::ControlMessageOwned. * Creating a SocketTimestamp enum for the SO_TS_CLOCK setsockopt for FreeBSD. * Fixing whitespace * Fixing CI error on cfg attributes * Removing legacy doc attributes * Formatting cleanup * Updating changelog * Adding tests for new TsClock setsockopt enum and the two new packet timestamp control messages for FreeBSD * Replacing an assert_eq with an assert in new tests. * Removing qemu ignore for new FreeBSD tests * Giving new FreeBSD timestamping tests each a unique socket * Updating monotonic packet timestamp test to account for monotonicity * Moving test ports again to line up with changes in #2196 * Attempting checks again * Wrapping ptr::read_unaligned calls in unsafe blocks
Yeah, if we do something like this, we should document it well.
Indeed, the old one is clearer, but will we also do this to IP address? It should be something like this if we only do this to ports: let port = PORT_ALLOCATOR.next();
let addr = SockAddrV4::from_str(&format!("127.0.0.1:{port}")).unwrap(); |
Either way would work. What do you think is best? |
I think we should add a port allocator because manually maintaining the state is too hard, and conflicts are difficult to catch as well since tests are running concurrently. For the readability issue, it should not be that bad if we only do this to ports:
BTW, is there any guideline on when a test should be put in Tests for public Nix interfaces should all be placed under |
Not really. Nix has so many contributors; it seems like each has placed tests according to his preference. For me personally I have a rule that I like to use, but it doesn't really apply to Nix.
👍 |
Should we merge this PR? I think it is ok to do that port allocator thing in the future |
Yeah, I think so. Would you approve it please? |
What does this PR do
Checklist:
CONTRIBUTING.md