-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add cloning to MultiplayerPlaylistItem
, and tests using fake data
#32372
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
Conversation
@@ -1,6 +1,7 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<Import Project="..\osu.TestProject.props" /> | |||
<ItemGroup Label="Package References"> | |||
<PackageReference Include="Bogus" Version="35.6.2" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably gonna need @peppy's ok for the inclusion on this. It seems mostly legit on a quick look although there's some DRM-shaped stuff happening because the library has paid extensions included, and also some other somewhat dodgy stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One alternative that just came to my mind, is I can replace it with
public MultiplayerPlaylistItem Clone()
{
MultiplayerPlaylistItem clone = (MultiplayerPlaylistItem)MemberwiseClone();
clone.RequiredMods = RequiredMods.ToArray();
clone.AllowedMods = AllowedMods.ToArray();
return clone;
}
And we can live without tests under the premise that MemberwiseClone()
has been properly tested by .NET itself. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah but the tests also test MPI -> PI -> MPI... Think I'll replace with MemberwiseClone() in any case. :/
I hope these classes can be interfaced or merged, at some point...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's okay since it's limited to the tests project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no major objections I guess, requesting @peppy review for the new library inclusion
This is intended to fix a separate issue I've found server-side, where
MultiplayerPlaylistItem
's are cloned manually: https://github.com/ppy/osu-server-spectator/blob/8c3c87039659d623870f77bf7bf3d1610f202f85/osu.Server.Spectator/Hubs/Multiplayer/MultiplayerQueue.cs#L254-L263Given that this has come up a few times now, I think it's best to add cloning and supporting tests.
For testing, I've added the
Bogus
library. It seems well supported and MIT. Its strict mode tells us when a property isn't being touched by the test, which should catch any missed updates to these methods.The checksum change is irrelevant to the above fix - it was noticed separately as a consequence of the tests.