Repository navigation
Conversation
added 6 commits
September 9, 2026 13:38
…ayout The test compared neighboringFields(...).filterIsInstance<PASSENGER>().first(), while Board.pickupPassenger decrements the first neighbour that additionally satisfies direction == opposite() && passenger > 0. On the ~1.5% of randomly generated boards where the ship borders two passenger fields those are different fields, so the assertion failed for roughly 0.7% of all runs. Sum the passengers of all neighbouring passenger fields instead, which holds regardless of which field the implementation picks. Also expect the ship to gain exactly one passenger, matching the implementation (passengers++) rather than relying on the field happening to hold exactly one.
The input "<protocol></protocol>" made the receive thread started by the XStreamClient constructor hit the end of the stream, treat it as a lost connection and close the client. When it won that race against the sending thread, sendObject dropped the packet and the written XML stayed empty. Leave the object stream open, as clientReceivePacketTest already does, so the reader blocks in NonEndingByteArrayInputStream instead of disconnecting.
sendCustomData writes straight to the socket, so a connection the peer has already aborted surfaces as SocketException on Linux but as a plain IOException from NioSocketImpl on Windows, which slipped past the catch.
NewClientListener.lastUsedPort was a public static field that every test reset to 0 before starting its own lobby and then polled until it turned non-zero. With several lobbies alive in one JVM they overwrite each other's port, and a test can end up connecting to a foreign server or to port 0. Keep the bound port on the listener instead and expose it through ClientManager.port and Lobby.port. Since startSocketListener binds the socket synchronously within start(), the port is known as soon as Lobby.start() returns, so the waitForServer() busy-loops in RealServerTest and TestLobby are gone as well.
sc.server.Configuration is global state, but LobbyRequestTest set the password to "Bobby" while TestLobby and RealServerTest set "TEST_PASSWORD". Kotest runs specs in parallel, so whichever spec wrote last decided whether an authentication elsewhere succeeded. Configure the shared PASSWORD constant everywhere and let the negative test authenticate with a deliberately different literal instead of relying on no password being set yet. Its duplicated name is fixed along the way: the second "wrong password" test actually checks the correct one.
GameState generates a random Board, so the ships in the "Result XML" scenario occasionally end up next to a passenger field and pick one up, which adds POINTS_PER_PASSENGER to the score and broke the hardcoded expectation. Measured over 5000 runs of the scenario: the state after two moves is always ONE 3 / TWO 2 points without passengers, after the third move Alice carries a passenger in 3.2% of the boards (8 points), and the winner is ONE every time, so only the score parts need to give way. Derive the expected points from the ship's actual passenger count instead of pinning the board, keeping the advance points asserted exactly. Verified by replaying the assertion 3000 times, 119 of them with a picked-up passenger.
| usedPort); | ||
| this.serverSocket = new ServerSocket(configuredPort, 0, bindAddr); | ||
| this.port = this.serverSocket.getLocalPort(); | ||
| logger.info("Listening on port {} for incoming connections.", this.port); |
Member
There was a problem hiding this comment.
Why does this log this.port and not the configuredPort? What is the difference between the two?
Comment on lines
+24
to
+32
| /** | ||
| * An ObjectStream that is opened but never closed (to be used with XStream). | ||
| * | ||
| * It must not contain the closing tag: the receive thread started by the | ||
| * {@link XStreamClient} constructor would hit the end of the stream, | ||
| * treat it as a lost connection and close the client - | ||
| * racing against the sending thread, which then silently drops its packet. | ||
| */ | ||
| private static final String OPEN_OBJECT_STREAM = "<protocol>"; |
Member
There was a problem hiding this comment.
Which one of the flaky tests does this solve?
| lobby.onRequest(dummy, PacketCallback(AuthenticateRequest(pwd))) | ||
| test("correct password") { | ||
| Configuration.set(Configuration.PASSWORD_KEY, PASSWORD) | ||
| lobby.onRequest(dummy, PacketCallback(AuthenticateRequest(PASSWORD))) |
Member
There was a problem hiding this comment.
The test changed from a wrong password to a correct password test, why?
| } catch(_: SocketException) { | ||
| } catch(_: IOException) { | ||
| // The server may have dropped the connection already; | ||
| // Windows surfaces that as a plain IOException rather than a SocketException. |
Member
There was a problem hiding this comment.
Only windows? Is this OS specific?
Comment on lines
-55
to
-60
| private fun waitForServer() { | ||
| while (NewClientListener.lastUsedPort == 0) { | ||
| Thread.yield() | ||
| } | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Why is this no longer needed?
This branch has not been deployed
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
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.
Should fix tests failing eratically