test: add MTE network and multiplayer integration tests#5325
Open
agent-refr wants to merge 7 commits into
Open
test: add MTE network and multiplayer integration tests#5325agent-refr wants to merge 7 commits into
agent-refr wants to merge 7 commits into
Conversation
Salvaged and cleaned up from PR MovingBlocks#5304 (Antigravity DI exploration). All debug logging removed, imports cleaned, tests restructured. - ClientNetworkStateTest: verifies client NetworkSystem mode and NetworkSubsystem presence - ClientSystemUpdateTest: registers a system on a client and verifies it receives update ticks through the MTE game loop - TwoClientChatTest: two-client chat propagation via ChatMessageEvent broadcast through the LISTEN_SERVER network stack Also adds Engine-Testing-Patterns.md documenting MTE patterns, the network event registration problem (inner-class events don't replicate), context isolation, and Gradle execution tips. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a testing guide and three ModuleTestingEnvironment integration tests that exercise client creation, client-side subsystem updates, and multi-client chat propagation under a listen-server network environment. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/Engine-Testing-Patterns.md`:
- Around line 79-85: The docs incorrectly state that ChatMessageEvent is a
broadcast event; update the wording to reflect that the ChatMessageEvent class
is annotated with `@OwnerEvent` (not `@BroadcastEvent`) so tests shouldn’t assume
broadcast semantics — locate references to ChatMessageEvent in the docs (e.g.,
the example using clientEntity.send(new ChatMessageEvent(...))) and change the
description to say it is owner-only/owner-annotated (`@OwnerEvent`) and not
replicated as a broadcast, and suggest using an engine-registered
`@BroadcastEvent` if true broadcast replication is required.
- Around line 98-107: The docs currently show registering a probe twice which
causes duplicate event delivery; update the example to only register the probe
via ComponentSystemManager.register (e.g., remove the
EventSystem.registerEventHandler call), and note that
ComponentSystemManager.register(probe) already registers
BaseComponentSystem-derived systems as event handlers so calling
EventSystem.registerEventHandler(probe) (as shown for MyProbe) is unnecessary
and should be omitted.
In
`@engine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java`:
- Around line 40-79: The test currently picks an arbitrary sender (senderClient)
from hostNetwork.getPlayers(), so client1Ctx is unused; change the sender
selection to specifically use client1Ctx: obtain the clientInfo for the local
client created in client1Ctx (via its ClientComponent.clientInfo) and then find
the matching Client in hostNetwork.getPlayers() whose
client.getEntity().getComponent(ClientComponent.class).clientInfo equals that
client1 clientInfo, assign that matching Client to senderClient, and use it when
building senderInfo and sending ChatMessageEvent so the test actually verifies
client1→client2 chat propagation.
- Around line 64-69: The ChatProbe is being registered twice which can cause
duplicate event delivery; remove the redundant direct EventSystem registration
by deleting the call to
client2Ctx.get(EventSystem.class).registerEventHandler(probe) and rely solely on
client2Csm.register(probe) (where ChatProbe is the class with `@ReceiveEvent`
handlers) so the probe is only registered once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0953e51e-430e-4ff6-ab84-82b40ed75685
📒 Files selected for processing (4)
docs/Engine-Testing-Patterns.mdengine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientNetworkStateTest.javaengine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientSystemUpdateTest.javaengine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java
Document the PathManager/CoreRegistry singleton save/restore pattern, the @tempdir cleanup ordering gotcha, and why missing tearDown causes non-deterministic cross-class test failures in CI. Prompted by a real CI failure: PathManagerTest mutated PathManager without restoring, causing ServerConnectListManagerTest to fail with NoSuchFileException on Jenkins but not locally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All tests that call helper.createClient() are subject to connection timeouts in resource-constrained CI environments. This matches the existing pattern in ExampleTest where all client-creating methods are tagged @tag("flaky"). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
engine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java (2)
65-70:⚠️ Potential issue | 🟡 MinorRemove the duplicate event-handler registration.
client2Csm.register(probe)should be the single registration path for the component system; registering the same handler directly withEventSystemcan double-deliverChatMessageEvents.🧹 Proposed fix
ChatProbe probe = new ChatProbe(); ComponentSystemManager client2Csm = client2Ctx.get(ComponentSystemManager.class); client2Csm.register(probe); - // Also register with EventSystem for post-init registration - client2Ctx.get(EventSystem.class).registerEventHandler(probe);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java` around lines 65 - 70, The ChatProbe is being registered twice which causes duplicate event delivery; remove the direct EventSystem registration and rely solely on the component system registration. Specifically, keep the call client2Csm.register(probe) (ComponentSystemManager.register) and delete the line that calls client2Ctx.get(EventSystem.class).registerEventHandler(probe) so ChatProbe is only registered once.
72-80:⚠️ Potential issue | 🟠 MajorBind the sender to client 1 before sending the message.
Line 74 picks an arbitrary connected host client, so this may validate host-to-client delivery from either client rather than the intended client1-to-client2 path.
🎯 Proposed direction
- Client senderClient = hostNetwork.getPlayers().iterator().next(); + EntityRef client1Info = findLocalClientInfo(client1Ctx); + Client senderClient = null; + for (Client client : hostNetwork.getPlayers()) { + EntityRef candidateInfo = client.getEntity().getComponent(ClientComponent.class).clientInfo; + if (candidateInfo.equals(client1Info)) { + senderClient = client; + break; + } + } + assertNotNull(senderClient, "Client 1 should be registered on the host"); EntityRef senderInfo = senderClient.getEntity().getComponent(ClientComponent.class).clientInfo;Add a small helper in the test class:
private static EntityRef findLocalClientInfo(Context clientContext) { NetworkSystem network = clientContext.get(NetworkSystem.class); for (Client client : network.getPlayers()) { ClientComponent component = client.getEntity().getComponent(ClientComponent.class); if (component != null && component.local) { return component.clientInfo; } } throw new AssertionError("Local client info should exist"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java` around lines 72 - 80, The test currently picks an arbitrary sender from hostNetwork.getPlayers(), which can pick the wrong client; add and use a helper to find the local client's ClientComponent.clientInfo and bind the sender to client1 explicitly. Implement a private helper like findLocalClientInfo(Context clientContext) that iterates network.getPlayers(), checks ClientComponent.component.local, and returns component.clientInfo (throw an assertion if missing), then in TwoClientChatTest replace the arbitrary senderClient/senderInfo selection with a call that obtains the client1 context and uses findLocalClientInfo(client1Context) as the senderInfo when constructing ChatMessageEvent and sending to other clients.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@engine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java`:
- Around line 65-70: The ChatProbe is being registered twice which causes
duplicate event delivery; remove the direct EventSystem registration and rely
solely on the component system registration. Specifically, keep the call
client2Csm.register(probe) (ComponentSystemManager.register) and delete the line
that calls client2Ctx.get(EventSystem.class).registerEventHandler(probe) so
ChatProbe is only registered once.
- Around line 72-80: The test currently picks an arbitrary sender from
hostNetwork.getPlayers(), which can pick the wrong client; add and use a helper
to find the local client's ClientComponent.clientInfo and bind the sender to
client1 explicitly. Implement a private helper like findLocalClientInfo(Context
clientContext) that iterates network.getPlayers(), checks
ClientComponent.component.local, and returns component.clientInfo (throw an
assertion if missing), then in TwoClientChatTest replace the arbitrary
senderClient/senderInfo selection with a call that obtains the client1 context
and uses findLocalClientInfo(client1Context) as the senderInfo when constructing
ChatMessageEvent and sending to other clients.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0f5b89a-67a5-46b6-9fac-e9a1d66b87a2
📒 Files selected for processing (3)
engine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientNetworkStateTest.javaengine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientSystemUpdateTest.javaengine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- engine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientSystemUpdateTest.java
… registration - ChatMessageEvent is @OwnerEvent, not @BroadcastEvent — fix docs and test Javadoc - Remove redundant EventSystem.registerEventHandler() call in test and docs example — ComponentSystemManager.register() already handles it - Remove unused client1Ctx variable and unused EventSystem import - Fix test message to reflect what's actually tested (host-to-client, not client1-to-client2) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
engine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java (1)
25-30:⚠️ Potential issue | 🟡 MinorBind the sender, or narrow the test name/scope.
Line 70 still picks an arbitrary host-side client, so the sender can be the same client as the probed receiver. Either derive the sender from the first created client context, or rename the test/Javadoc to describe host-to-client delivery rather than “between clients.”
Also applies to: 40-40, 70-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java` around lines 25 - 30, The test currently chooses an arbitrary host-side client as the sender, which allows the sender and probed receiver to be the same; update TwoClientChatTest so the host-side sender is explicitly derived from the first created client context (instead of the arbitrary pick used when creating the ChatMessageEvent) — locate the code that selects the host-side client entity for sending the ChatMessageEvent and bind it to the first client context created in the test setup (or alternatively change the test/Javadoc/test name to indicate this is host-to-client delivery rather than “between clients”); ensure references to ChatMessageEvent and the LISTEN_SERVER verification use the newly bound sender.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@engine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java`:
- Around line 78-80: The test sets a too-short real-time safety cap which can
trigger MainLoop.runWhile() safety timeout before the intended game-time wait;
update the test around helper.setSafetyTimeoutMs(...) and helper.runUntil(...)
so the safety timeout is not shorter than the normal propagation wait (e.g.,
remove the 10000ms override or increase it to a larger value such as 60000ms),
ensuring probe.received has time to become true without hitting
MainLoop.runWhile()'s safety timeout.
---
Duplicate comments:
In
`@engine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java`:
- Around line 25-30: The test currently chooses an arbitrary host-side client as
the sender, which allows the sender and probed receiver to be the same; update
TwoClientChatTest so the host-side sender is explicitly derived from the first
created client context (instead of the arbitrary pick used when creating the
ChatMessageEvent) — locate the code that selects the host-side client entity for
sending the ChatMessageEvent and bind it to the first client context created in
the test setup (or alternatively change the test/Javadoc/test name to indicate
this is host-to-client delivery rather than “between clients”); ensure
references to ChatMessageEvent and the LISTEN_SERVER verification use the newly
bound sender.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7201417b-5195-42ac-844b-5cd689e34d8c
📒 Files selected for processing (2)
docs/Engine-Testing-Patterns.mdengine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java
✅ Files skipped from review due to trivial changes (1)
- docs/Engine-Testing-Patterns.md
…ng diagnostics - Match client 1 to its host-side representation via ClientComponent entity ID so the sender identity is explicit, not arbitrary - Bump safety timeout from 10s to 45s — test takes ~27s locally, 10s was too low for CI - Add SLF4J timing logs at each milestone (client connect, registration, send, receive) to help diagnose flaky CI failures - Fix Javadoc to describe @OwnerEvent semantics correctly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds new ModuleTestingEnvironment (MTE) multiplayer/network integration tests and accompanying documentation to help validate and guide engine-level network behavior under headless listen-server scenarios.
Changes:
- Add 3 MTE integration tests covering client network state, client update ticks, and two-client chat event propagation.
- Add new documentation page describing MTE testing patterns and common pitfalls for network event replication.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| engine-tests/src/test/java/org/terasology/engine/integrationenvironment/TwoClientChatTest.java | Adds a two-client listen-server integration test for chat event replication using ChatMessageEvent. |
| engine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientSystemUpdateTest.java | Adds a client-side integration test ensuring UpdateSubscriberSystem receives ticks in the MTE loop. |
| engine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientNetworkStateTest.java | Adds client integration tests validating NetworkSystem mode and presence of NetworkSubsystem. |
| docs/Engine-Testing-Patterns.md | Documents engine/MTE testing patterns, network event registration gotchas, and isolation guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tics TwoClientChatTest: remove fragile cross-engine entity ID matching (IDs aren't stable across engines), dead permission-grant block, and the lowered safety timeout — use the 60s default instead. ClientNetworkStateTest: use assertInstanceOf instead of bare cast. All three tests: add JUnit 5 TestReporter.publishEntry() for structured timing data (client_connect_ms, total_ms, etc.) that appears in JUnit XML reports for Jenkins API access. Engine-Testing-Patterns.md: add TestReporter diagnostics section, fix NetworkMode qualifier in table. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
docs/Engine-Testing-Patterns.mddocumenting MTE testing patterns, including the network event registration problem (inner-class@BroadcastEvent/@OwnerEvent/@ServerEventevents don't replicate) and context isolationTest plan
ClientNetworkStateTest— verifies client NetworkSystem mode (CLIENT) and NetworkSubsystem presenceClientSystemUpdateTest— registers UpdateSubscriberSystem on client, verifies it receives ticksTwoClientChatTest— two clients connect, chat message broadcast via ChatMessageEvent propagates between themRelated
NetworkEventSystemWrappedfix from the DI merge (b353187)