Skip to content

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
wants to merge 16 commits into
base: develop-2.0.0
Choose a base branch
from

Conversation

NoelStephensUnity
Copy link
Collaborator

@NoelStephensUnity NoelStephensUnity commented May 3, 2025

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:

  • Uses a client-server network topology
    • No point in re-running these as they will have already run multiple times on multiple platforms before testing against the CMB server.
  • Has overridden UseCmbService and is returning false.
    • The test has yet to be converted and so no point in even attempting to run it.
    • This also includes the TODO: [CmbServiceTests] to help track all tests that need to be converted or reviewed to determine if they need to be converted.
  • Is not derived from NetcodeIntegrationTest.
    • Many of these (not all) don't need to run again, but they all have the 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:

        // TODO: [CmbServiceTests] Adapt to run with the service
        protected override bool UseCMBService()
        {
            return false;
        }

For classes that do not derive from NetcodeIntegrationTest:

        [OneTimeSetUp]
        public void OneTimeSetup()
        {
            // TODO: [CmbServiceTests] if this test is deemed needed to test against the CMB server then update this test.
            NetcodeIntegrationTestHelpers.IgnoreIfServiceEnviromentVariableSet();
        }

(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 in NetcodeIntegrationTest.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

  • Includes integration test adjustments.
  • Includes NetcodeIntegrationTest related adjustments.
  • No documentation changes or additions were necessary.

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.

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.
removing unused property.
renaming a local variable for consistency in naming.
removing unused overridden virtual method.
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.
Fixing the wording on the USE_CMB_SERVICE warning log if it is an invalid bool string.
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.
Fixing misspelled GetServiceEnvironmentVariable.
Fixing comment for grammar and clarity.
Missed one reference to GetServiceEnviromentVariable
@NoelStephensUnity NoelStephensUnity requested a review from EmandM May 5, 2025 01:37
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.
removing the OnPreInitializeConfiguration()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant