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

Fix intermittent failure in screen navigation tests #22221

Merged
merged 1 commit into from
Jan 15, 2023

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Jan 15, 2023

TearDown : System.AggregateException : One or more errors occurred. (Cannot invoke BeginPlaying when already playing)
  ----> System.InvalidOperationException : Cannot invoke BeginPlaying when already playing
Data:
  Sentry:Mechanism: AppDomain.UnhandledException
  Sentry:Handled: False
--TearDown
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at osu.Framework.Extensions.TaskExtensions.WaitSafely(Task task)

https://github.com/frenzibyte/osu/actions/runs/3913119373/jobs/6688620934
https://teamcity.ppy.sh/test/2989819594862160493?currentProjectId=Osu&expandTestHistoryChartSection=true

See #22220 for more information. I've included two changes here, one for handling restarts on TestPlayer instances by enforcing EndPlaying before exiting screen (extra safety in addition to disposal logic), and another for the failure in TestSceneScreenNavigation specifically, since that one is based of a full game instance, which would use SoloPlayer instead of TestPlayer.

Not fully confident on the test-side fix (dumpster fire for sure), but I can't think of any better way for it so...

For reproduction, simply add a task delay in SubmittingPlayer.OnExitting before performing score submission.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Mixed feelings on this. I could live with the fix, although the fact that now all player tests differ in behaviour to the actual game - which also includes TestScenePlayerScoreSubmission - worry me a little bit.

@@ -47,7 +47,7 @@ public abstract partial class SpectatorClient : Component, ISpectatorClient
/// <summary>
/// Whether the local user is playing.
/// </summary>
protected bool IsPlaying { get; private set; }
protected internal bool IsPlaying { get; private set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this modifier change for? Nothing new appears to be using this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Leftover change (boring details is: wanted the test to make sure player called BeginPlaying before ending it, but realised I could guarantee it by just ensuring player fully loaded).

@peppy
Copy link
Member

peppy commented Jan 15, 2023

In the interest of keeping momentum and reducing developer frustration I'm okay with this. It's very clear that we need to address this in a better way, but that can wait, in terms of priorities (tracking via #22220).

@peppy peppy merged commit a397b9c into ppy:master Jan 15, 2023
@frenzibyte frenzibyte deleted the fix-screen-navigation-test-failure branch January 15, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants