test: all runtime tests passing on consoles #1647
Conversation
This is a "quasi-fix" where I had to add the ability to run NetworkTransform in the FixedUpdate in order to get the NetworkTransform tests all passing when running in a stand alone test runner build.
…Technologies/com.unity.netcode.gameobjects into fix/console-tests-failing
moving the HasTimedOut check to the end of each while loop to assure that any possible edge case scenario where it finishes just as it would be timing out is not missed. An example would be the condition being checked is validated but it lands right on the timeout period. The test would have been a success, but timedOut would have been set too.
This update collapses all of the time out code into the BaseMultiInstanceTest with an added method WaitForConditionOrTimeOut that leverages from an added TimeOutHelper class.
Needed some MULTIPLAYER_TOOLS helpers that I accidentally excluded.
removing some of the Run calls and beginning to convert over some of the problematic tests to just yield return the conditional check itself to avoid potential timing issues when checking the condition and yielding to the current test group's newly created coroutine.
fixed issue with WhenListContainsManyLargeValues_OverflowExceptionIsNotThrown count comparison value. Updated removed FixedString32Test by removing yield return that was not needed.
Still trying to get rid of IL2CPP error
This test was failing on some of the console platforms. Made adjustments to avoid timing issues.
This reverts commit a2f6fca.
…passing-on-consoles
|
|
||
| // This really should never fail as it should timeout first | ||
| Assert.True(allClientsCompleted, "Not all client side NetworkVariables reported they were updated with target value!"); | ||
| yield return WaitForConditionOrTimeOut((c) => |
There was a problem hiding this comment.
Something to consider here: What this test is really trying to do is wait for messages to be sent and received and then assert that the values have changed. Waiting for the values to change like this is kind of mixing the precondition with the assertion - if the assertion fails we wait forever.
In general, I would like to see us moving in the direction in our test code of waiting for distinct preconditions that are not part of what's being tested, and then asserting on the expected result. So in this case, using WaitForMessageOfType<SnapshotDataMessage> or WaitForMessageOfType<NetworkVariableDeltaMessage> (not sure if snapshots are on or not for this test) and then asserting that all the values have changed.
Reading this test as it's written now, if I were to interpret the written code as a "given-when-then" statement, I would word it as "Given an object with network variables on multiple clients, when all clients have changed the values of their variables, waiting for them to change doesn't time out".
I know this is a pretty pedantic comment, but I think moving toward smaller, more discreet tests that test only one thing and are very clear about what they're testing by what's inside the one assert, will ultimately make our test code better, cleaner, and easier to maintain.
There was a problem hiding this comment.
There are other places in this PR where this comment also applies, but I'm going to only mention it the once. It might be something to address later rather than now.
There was a problem hiding this comment.
(This comment, btw, is directed at myself as well. I've been guilty of this in the past, too. I've been trying to learn more about good test design and this is something I've learned has value over the last few months.)
There was a problem hiding this comment.
Actually, that is why I switched it over to the +new+ WaitForConditionOrTimeOut. It will wait for a condition to be reached and if the condition is never met it will stop waiting and the new assert below checks to see if it timed out or not. If not, it moves on, but if so then it asserts with a meaningful/unique message that makes troubleshooting a failed integration test easier to find.
| #if UNITY_EDITOR | ||
| [DontShowInTransportDropdown] | ||
| #endif | ||
| //#if UNITY_EDITOR |
There was a problem hiding this comment.
Was leaving this commented out an intentional change? SIPTransport doesn't feel like something that makes sense to be exposed in the editor, even in testproject.
There was a problem hiding this comment.
this was all changed...this PR will get deleted sometime this week. The solution was to create a derived class from NetworkTransport that is what SIPTransport derives from. In the editor it checks for anything that is a subclass of the testing transport class or if the class is derived from the testing transport.
NoelStephensUnity
deleted the
test/all-runtime-tests-passing-on-consoles
branch
This is a temporary Draft-PR to get all platforms building with both #1628 and #1644 merged together.