Socket test improvements - #108
Open
rnro wants to merge 6 commits into
Open
Conversation
Tests using sockets hard-coded loopback port numbers making them fragile and could cause spurious failures (or passes) depending on other activity on the system. `TCPClientHarness.init` also swallowed a server start failure with `try?`, so a bind failure surfaced later as an unexplained timeout. The three servers now bind port 0 and report back what the kernel gave them as `listeningPort`, and a start failure is reported where it happens. Both harnesses lose their port parameters, which takes port bookkeeping out of all 49 callsites. `discoverFreeLoopbackPorts(_:)` covers the peers that need a number before anything is bound to it, such as the UDP cross-binding and the connection tests. It discovers a free port by binding port 0 then releasing it. This is racey but has a small window which should be more resilient than the fixed ports. It takes several at once to avoid handing out the same port more than once.
`testTCPConnectionRefusedDeliversFailure` asserted the absence of `.ready` with an inverted `XCTestExpectation`, which XCTest can only satisfy by letting the whole timeout elapse. The test therefore paid its full timeout every run no matter how quickly the connection was refused, and only covered the window it waited out. Recording the observed state sequence and asserting `.ready` is absent from it catches the state anywhere before the failure and dumps the sequence when it does appear. It is also fast rather than fixed-cost (locally 0.002s against 5.003s).
`SplitSendServer` held its connection open with a one second sleep, which is a guess at how long the client needs, and kept the accepted descriptor and its thread alive past teardown. Blocking until `read` returns 0 is exact, and it is the idiom `TCPEchoServer` in the same file already uses. The `gap` between the two segments stays, and is now documented as load-bearing: it is what makes the client process the first segment alone, which is the condition `testTCPReceiveAtLeastSpanningTwoSegments` reproduces, and shrinking it lets the writes coalesce so the test passes without exercising the bug. Both waits in `testTCPNoBusyLoopAfterEOF` are documented for the same reason -- the second one is the measurement, since no read event firing is only observable as an absence over an interval.
`testCreateInterfaceWithTooManySockets` provoked a socket-creation failure by opening sockets until the kernel refused. That was slow (locally 17s), and it exhausts a machine-wide resource: while it runs, anything else needing a descriptor can fail, so it causes spurious failures in unrelated tests and in other processes. Lowering this process's own `RLIMIT_NOFILE` to what it is already using makes the very next `socket()` fail, which is the condition the test wants, without affecting anything else, and is fast (locally under a millisecond). The limit is restored before the assertion runs, because XCTest opens files to report a failure. Reading and lowering the limit both stop the test on failure rather than recording and continuing: a failed `getrlimit` leaves the value zeroed, and lowering from that would clamp the hard limit to zero, which an unprivileged process can never raise again.
agnosticdev
approved these changes
Aug 21, 2026
|
|
||
| func testEmptyPayload() { | ||
| let harness = UDPLoopbackHarness(basePort: 10950) | ||
| let harness = UDPLoopbackHarness() |
Collaborator
There was a problem hiding this comment.
This is nice, thank you!
PayasR
approved these changes
Aug 25, 2026
| import Musl | ||
| internal import Logging | ||
| #elseif canImport(os) | ||
| internal import os |
Contributor
There was a problem hiding this comment.
Is this just an empty #elseif branch now?
tfpauly
approved these changes
Aug 26, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This PR aims to make Socket tests faster and more stable, no source changes.
listeningPort.discoverFreeLoopbackPorts). This is still racy, but avoids most collisions and issues when hanging tests have the port.TCPClientHarness.initreports a start failure instead of dropping it withtry?, so a bind failure no longer surfaces later as an unexplained timeout.RLIMIT_NOFILEinstead of opening sockets until the kernel refuses. This was probably a cause of a reasonable amount of test flakiness and was slow. (locally 17s to under a millisecond).