fix: UTP adapter tests failing on consoles#1372
Conversation
A lot of tests were waiting on the Connect event on the server side as a proxy of the connection being fully established. But that's not correct. A Connect event on the server does not mean that the UTP Connection Accept message has been received and processed by the client. This lead to some timing issues where we'd try to e.g. send data on the client side before the connection is fully established.
| } | ||
|
|
||
| // Check that all clients received the correct event. | ||
| Assert.True(m_ClientsEvents.All(evs => evs[0].Type == NetworkEvent.Connect)); |
There was a problem hiding this comment.
nit: Just a tip, you can use Assert.That and specify constraints. This will ensure that you get a relevant error message when the test fails, rather than just "Expected true, got false".
https://docs.nunit.org/articles/nunit/writing-tests/assertions/assertion-models/constraint.html
There was a problem hiding this comment.
I didn't know about that. Thanks for pointing it out!
| // Check that all clients got a Disconnect event. | ||
| Assert.True(m_ClientsEvents.All(evs => evs.Count == 2)); | ||
| Assert.True(m_ClientsEvents.All(evs => evs[1].Type == NetworkEvent.Disconnect)); | ||
| Assert.That(m_ClientsEvents.All(evs => evs.Count == 2)); |
There was a problem hiding this comment.
Sorry, I should have been more precise with that comment. If you straight up replace True with That, it will behave exactly the same.
To get relevant error messages, you need to use it in the following way:
Assert.That(m_ClientEvents, Has.All.EqualTo(your constraint here);
That way, it will be able to mark which specific collection items are not matching your constraint, and should also display the actual value vs the expected value.
There was a problem hiding this comment.
Has.All.EqualTo, or any other Has.All method that you prefer
There was a problem hiding this comment.
Ah right, I see. In that case I'm not sure it's really worth it in that particular case. While the first check (events.All(evs => evs.Count == 2)) can be expressed in that style, it's immediately followed by another similar check that I'm not sure can be written using such constraints. I prefer the extra consistency here of having all similar assertions using a similar style.
3 participants
Note that this is only a tentative fix. But it's a definitive improvement to the adapter tests, so should help with the stability of the tests whether it actually fixes the console tests or not.
The issue was that a lot of tests were waiting on the
Connectevent on the server side as a proxy of the connection being fully established. But that's not correct. AConnectevent on the server does not mean that the UTPConnectionAcceptmessage has been received and processed by the client. This potentially lead to some timing issues where we'd try to e.g. send data on the client side before the connection is fully established.This PR tentatively fixes MTT-1551 and MTT-1552.
Changelog
com.unity.netcode.adapter.utp
N/A (Only changes are to tests.)
Testing and Documentation