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

Refactor song select test methods to be resilient to slow realm callbacks #17169

Merged

Conversation

peppy
Copy link
Member

@peppy peppy commented Mar 8, 2022

@bdach
Copy link
Collaborator

bdach commented Mar 8, 2022

I've ran this locally with the diff from the gist in OP and got this sort of error back once or twice:

TearDown : System.AggregateException : One or more errors occurred. (Specified argument was out of the range of valid values. (Parameter 'Get from RealmResults index:5 beyond range of:0'))
  ----> System.ArgumentOutOfRangeException : Specified argument was out of the range of valid values. (Parameter 'Get from RealmResults index:5 beyond range of:0')
--TearDown
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.Wait()
   at osu.Framework.Extensions.TaskExtensions.WaitSafely(Task task)
   at osu.Framework.Testing.TestScene.checkForErrors()
   at osu.Framework.Testing.TestScene.RunTestsFromNUnit()
--ArgumentOutOfRangeException
   at Realms.ResultsHandle.GetValueAtIndex(Int32 index, Realm realm)
   at Realms.RealmResults`1.GetValueAtIndex(Int32 index)
   at osu.Game.Screens.Select.BeatmapCarousel.beatmapSetsChanged(IRealmCollection`1 sender, ChangeSet changes, Exception error) in /home/dachb/Dokumenty/opensource/osu/osu.Game/Screens/Select/BeatmapCarousel.cs:line 261
   at osu.Game.Database.RealmObjectExtensions.<>c__DisplayClass11_1`1.<QueryAsyncWithNotifications>b__2(Object _) in /home/dachb/Dokumenty/opensource/osu/osu.Game/Database/RealmObjectExtensions.cs:line 291

Not sure whether it's a separate issue yet.

@bdach
Copy link
Collaborator

bdach commented Mar 8, 2022

So the crash I'm seeing boils down to the notification subscription itself, it seems:

2022-03-08-180204_504x310_scrot

Looks like the ChangeSet and the RealmResults are out of sync, presumably because of the sleep added in the gist to repro the other failures - I don't know how to reasonably explain it otherwise, other than "upstream bug". That would make this crash out of scope here I think, but also means that there's possibly a fundamental unsafety in posting the callback as there is apparently no guarantee that the ChangeSet is still going to be valid once the posted callback is actually executed... Which is worrying but still out of scope.

@smoogipoo smoogipoo merged commit af44239 into ppy:master Mar 9, 2022
@peppy peppy deleted the fix-song-select-test-failures-realm-callback branch March 10, 2022 05:10
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.

None yet

3 participants