Skip to content
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

Add unit tests for SceneMultiplayer #97607

Merged

Conversation

pafuent
Copy link
Contributor

@pafuent pafuent commented Sep 29, 2024

This PR aims to help "fix" #43440

I implemented as many test I could without using an actual MultiplayerPeer (like ENetMultiplayerPeer) or writing a MultiplayerPeer mock.
Using ENetMultiplayerPeer in this tests will imply that changes on ENetMultiplayerPeer could break tests on SceneMultiplayer, which is not desirable. Also I'm not sure if I'll have enough control on how ENetMultiplayerPeer behaves to fully test SceneMultiplayer.
For the second option, I'll wait until a decision is made on #95224. If a mocking library is available, I'll revisit SceneMultiplayer to add more unit tests.

Also I'm fixing a small typo on SceneMultiplayer docs.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work 🚀

This PR aims to help "fix" godotengine#43440

Also fixing a small typo on `SceneMultiplayer` docs.
@pafuent pafuent force-pushed the firsts_multiplayer_unit_tests branch from 5c7057f to e376c4f Compare September 30, 2024 00:54
Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Docs change is fine, as are the tests, built and tested locally on Windows.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Oct 1, 2024
@akien-mga akien-mga merged commit 49700c3 into godotengine:master Oct 1, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants