-
Notifications
You must be signed in to change notification settings - Fork 446
fix: cmb service compatibility updates and fixes #3437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
NoelStephensUnity
wants to merge
16
commits into
develop-2.0.0
Choose a base branch
from
fix/cmb-service-compatibility-update-fixes
base: develop-2.0.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
fix: cmb service compatibility updates and fixes #3437
NoelStephensUnity
wants to merge
16
commits into
develop-2.0.0
from
fix/cmb-service-compatibility-update-fixes
Conversation
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
This fixes some compatibility issues with the initial total number of clients count being always +1 as opposed to just checking for the scenario where an integration test is connecting to a CMB server and has the `NetcodeIntegrationTest.NumberOfClients` set to zero (which in this case we have to create at least 1 client). This also updates the `m_NumberOfClients` when this scenario is detected. This also fixes some issues with allowing the `NetcodeIntegrationTest `to still create and assign `m_ServerNetworkManager`. The fix replaces all pertinent areas within `NetcodeIntegrationTest` that was directly using `m_ServerNetworkManager` with the returned value of `NetcodeIntegrationTest.GetAuthorityNetworkManager`.
Fixing some issues with getting the session owner prior to actually starting and connecting to a service. Added a better log message that includes the actual name of the NetworkManager when a prefab cannot be found.
Fixing some issues with DistributedAuthorityCodecTests creating prefabs after having started the NetworkManager and issues with NetworkBehaviourTests having some bad/legacy technical debt in the initial design for those tests (particularly how it was checking if clients spawned objects).
This "should" constitute the remaining fixes for this PR's adjustments.
Fixing an issue where trying to parse for a bool I did not verify the alternative of no environment variable being set and the string value being "unset" as opposed to "false". This fixes that issue and if it is not a valid bool string the it logs a warning and disables CMB service testing.
Added some additional script to ignore anything that is not converted to run against the CMB server or any test that is configured to use a client-server network topology. Added OneTimeSetup to all non-NetcodeIntegrationTest derived tests that will also ignore the test until it is reviewed and determined it should be converted over or we do not need to run the test again since it will have already run multiple times on multiple platforms.
Adjusted 3 tests that showed potential for instabilities.
Reverting back to always incrementing clients when running against a CMB server. Excluding a few more tests I missed previously. Fixing RpcProxyMessageTesting to actually run and test against the correct expected counts.
Un-fixing the improperly named test class to avoid more PVP errors.
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.
This PR Includes
Backwards compatibility adjustments
The updates included in this PR assure any users leveraging from
NetcodeIntegrationTest
for their integration testing will not have tests fail due to skewed client counts.Initial setup updates
It also includes some minor improvements to the initial CMB server detection and with adjustments to the initial configuration.
Ignoring tests
This PR includes updates that will ignore any test that:
TODO: [CmbServiceTests]
to help track all tests that need to be converted or reviewed to determine if they need to be converted.TODO: [CmbServiceTests]
to help track all tests that need to be converted or reviewed to determine if they need to be converted.(This greatly reduces the time to complete the CMB Server CI pass)
Marking/Tracking "for review" tests
Assuring that every test that needs to be reviewed includes the
TODO: [CmbServiceTests]
using of the two ways:For classes that derive from
NetcodeIntegrationTest
:For classes that do not derive from
NetcodeIntegrationTest
:(This PR includes some tests that includes one of the two script segments but was was reviewed and determined it didn't need to be run against a CMB server)
TODO (pending review/discussion)
Session owner should connect first before all other clients.
There is still a little bit more to do in regards to getting the session owner after having created the
NetworkManager
instances but before starting them (see comments inNetcodeIntegrationTest.GetAuthorityNetworkManager
).While what is in place is working we should contemplate handling this particular scenario to avoid future instabilities as more integration tests are updated to run against a CMB server and while we wait for the CMB server updated connection sequence changes.
Changelog
NA - All internal testing and/or specific to testing.
Testing and Documentation
NetcodeIntegrationTest
related adjustments.Backport
Does not require a backport since these changes are specific to #3423 and getting NGO specific integration tests running against a CMB server instance.