[Communication] PR9: Added Mailbox tests#221
[Communication] PR9: Added Mailbox tests#221Joseph0120 wants to merge 1 commit intojoseph/communication_delay_PR8from
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 23 minutes and 6 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
|
🧱 Stack PR · Branch end (9 PRs in stack) Stack Structure:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive test suite for the Mailbox system, covering message delivery, latency scheduling, and receiver lifecycle management. Feedback suggests optimizing the tests by removing redundant initialization calls and cleanup logic, replacing brittle reflection with internal access, refactoring duplicated setup code, and ensuring test isolation by avoiding modifications to the global random state.
| Mailbox mailbox = mailboxObject.AddComponent<Mailbox>(); | ||
| InvokePrivateMethod(mailbox, "Awake"); |
There was a problem hiding this comment.
In Unity, AddComponent<T>() automatically calls the Awake method of the component if the GameObject is active. Manually invoking Awake via reflection here results in it being called a second time, which can lead to unintended side effects like double-initialization or multiple event subscriptions.
Mailbox mailbox = mailboxObject.AddComponent<Mailbox>();| foreach (GameObject gameObject in _createdObjects) { | ||
| if (gameObject != null) { | ||
| Object.DestroyImmediate(gameObject); | ||
| } | ||
| } | ||
|
|
||
| foreach (Mailbox mailbox in Object.FindObjectsByType<Mailbox>(FindObjectsSortMode.None)) { | ||
| if (mailbox != null) { | ||
| Object.DestroyImmediate(mailbox.gameObject); | ||
| } | ||
| } |
There was a problem hiding this comment.
The second loop in TearDown is redundant because all Mailbox objects created via the CreateMailbox helper are already tracked in the _createdObjects list and destroyed in the first loop. Additionally, it is good practice to call _createdObjects.Clear() after destruction to ensure the list is empty for subsequent tests if the test class instance is reused.
foreach (GameObject gameObject in _createdObjects) {
if (gameObject != null) {
Object.DestroyImmediate(gameObject);
}
}
_createdObjects.Clear();| SetPrivateField(mailbox, "_latencyJitterStdSeconds", -1f); | ||
| SetPrivateField(mailbox, "_uniformLatency", -2f); |
There was a problem hiding this comment.
Using hardcoded strings for private field names in reflection is brittle. If these fields are renamed in the Mailbox class, the tests will fail at runtime without compile-time errors. Consider making these fields internal and using [InternalsVisibleTo] to allow for safer, type-checked access in tests.
| private TestMailboxAgent CreateMailboxAgent(string name) { | ||
| GameObject agentObject = new(name); | ||
| _createdObjects.Add(agentObject); | ||
| agentObject.AddComponent<Rigidbody>(); | ||
| TestMailboxAgent agent = agentObject.AddComponent<TestMailboxAgent>(); | ||
| InvokePrivateMethod(agent, "Awake"); | ||
| return agent; | ||
| } | ||
|
|
||
| private TestMailboxInterceptor CreateMailboxInterceptor(string name) { | ||
| GameObject interceptorObject = new(name); | ||
| _createdObjects.Add(interceptorObject); | ||
| interceptorObject.AddComponent<Rigidbody>(); | ||
| return interceptorObject.AddComponent<TestMailboxInterceptor>(); | ||
| } | ||
|
|
||
| private TestMailboxCarrier CreateMailboxCarrier(string name) { | ||
| GameObject carrierObject = new(name); | ||
| _createdObjects.Add(carrierObject); | ||
| carrierObject.AddComponent<Rigidbody>(); | ||
| return carrierObject.AddComponent<TestMailboxCarrier>(); | ||
| } |
There was a problem hiding this comment.
The creation logic for different agent types is duplicated. Refactoring this into a generic helper method improves maintainability. This also removes the redundant manual Awake call in CreateMailboxAgent, consistent with the behavior of AddComponent on active GameObjects.
private T CreateTestAgent<T>(string name) where T : Component {
GameObject agentObject = new(name);
_createdObjects.Add(agentObject);
agentObject.AddComponent<Rigidbody>();
return agentObject.AddComponent<T>();
}
private TestMailboxAgent CreateMailboxAgent(string name) => CreateTestAgent<TestMailboxAgent>(name);
private TestMailboxInterceptor CreateMailboxInterceptor(string name) => CreateTestAgent<TestMailboxInterceptor>(name);
private TestMailboxCarrier CreateMailboxCarrier(string name) => CreateTestAgent<TestMailboxCarrier>(name);| } | ||
|
|
||
| private static float ComputeExpectedLatency(float baseLatency, float jitterStdSeconds, int randomSeed) { | ||
| Random.InitState(randomSeed); |
There was a problem hiding this comment.
Calling UnityEngine.Random.InitState modifies the global random state of the application. Doing this inside a helper method used for assertions can cause side effects in other tests. If possible, consider using a local System.Random instance for calculating expected values to ensure test isolation, provided the production code's logic can be matched.
Joseph0120
force-pushed
the
joseph/communication_delay_PR8
branch
from
0fe4232 to
dbe07cf
Compare
Joseph0120
force-pushed
the
joseph/communication_delay_PR9
branch
from
4dd5d2e to
e16e944
Compare
1 participant
Files Added:
MailboxTest.cs
Added simple test cases for Mailbox.cs.