chore: refactor session version serialization and configuration #3138
Conversation
…alization-and-configuration
| } | ||
| else | ||
| { | ||
| Assert.False(m_ClientWasDisconnected, "Client was disconnected when it was expected to connect!"); |
There was a problem hiding this comment.
If the above check is using WaitForConditionOrTimeOut(), could this test give a false positive, where it checks if it wasn't disconnected before the connect is processed?
There was a problem hiding this comment.
By the time it reaches that point the clients will have already been verified connected. That wait condition is waiting for the client to notify it had been disconnected.
| /// </remarks> | ||
| /// <param name="useValidSessionVersion">true = use valid session version | false = use invalid session version</param> | ||
| [UnityTest] | ||
| public IEnumerator ValidateSessionVersion([Values] bool useValidSessionVersion) |
There was a problem hiding this comment.
Out of interest, what is the benefit of combining both tests, rather than having two separate tests?
There was a problem hiding this comment.
Nothing really... it is a matter of preference in this particular case. Using TestFixtures and Values helps to reduce the script footprint... but in reality it is a matter of preference...
I personally prefer to have a test validating a specific feature/function of NGO to be under one test as opposed to several to avoid replicating code.
| /// Callback used to mock the scenario where a client has an invalid session version | ||
| /// </summary> | ||
| /// <returns><see cref="SessionConfig"/></returns> | ||
| private SessionConfig GetInavlidSessionConfig() |
There was a problem hiding this comment.
It might also be good to have a test on the new client connecting with SessionVersion + 1 and checking that they get downgraded?
There was a problem hiding this comment.
I thought about that but realized I would need to replicate that code in the NGO SDK to get a DAHost to handle this...which would sort of defeat that particular test.
The downgrade check should be a test that is validated in the service specific integration test that should verify this same existing functionality in this test plus the downgrade.
|
|
||
| internal uint SessionVersion; | ||
|
|
||
| public bool ServiceSideDistribution; |
There was a problem hiding this comment.
Does it make sense that ServiceSideDistribution is in the ConnectionApproved and ConnectionRequested messages?
There was a problem hiding this comment.
It only makes sense because the ConnectionRequestMessage uses it to define the client/package version (unifying the version to a single location) and is not actually serialized where the ConnectionApprovedMessage actually does serialize it when received by the client.
NoelStephensUnity
deleted the
chore/refactor-session-version-serialization-and-configuration
branch
This is a minor refactoring of #3132 in order to provide a more unified way to handle the session versioning (or "package version"). It includes a new
SessionConfigthat is configured upon approval and will be used to determine a session's supported features.Changelog
NA
Testing and Documentation