Skip to content

Fix ranked play test failures from adding null playlist items#37189

Merged
peppy merged 1 commit intoppy:masterfrom
bdach:fix-just-blatant-test-breakage
Apr 3, 2026
Merged

Fix ranked play test failures from adding null playlist items#37189
peppy merged 1 commit intoppy:masterfrom
bdach:fix-just-blatant-test-breakage

Conversation

@bdach
Copy link
Copy Markdown
Collaborator

@bdach bdach commented Apr 3, 2026

Example: https://github.com/ppy/osu/actions/runs/23900675414/job/69696255970?pr=37178#step:5:38

Regressed in #37172, cc @LiquidPL

Would fail in multiple tests. I'm not going to spend time figuring out exactly why, I'm just going to guess that not all tests bother to set up the relevant playlist items for the cards or whatever.

Some of the failing tests are flaky but not because the item here isn't sometimes null in those cases. It's always null, but the callbacks are probably scheduled or whatever and therefore have a chance to never run. Also some of the failures appear to cascade / spill from other tests as well.

Example: https://github.com/ppy/osu/actions/runs/23900675414/job/69696255970?pr=37178#step:5:38

Regressed in ppy#37172

Would fail in multiple tests. I'm not going to spend time figuring out
exactly why, I'm just going to guess that not all tests bother to set
up the relevant playlist items for the cards or whatever.

Some of the failing tests are flaky but not because the `item` here
isn't sometimes null in those cases. It's always null, but
the callbacks are probably scheduled or whatever and therefore have
a chance to never run. Also some of the failures appear to cascade /
spill from other tests as well.
@bdach bdach self-assigned this Apr 3, 2026
@bdach bdach added the area:test coverage Adds or modifies test coverage without adding any functionality. label Apr 3, 2026
@bdach bdach moved this from Inbox to Pending Review in osu! team task tracker Apr 3, 2026
@LiquidPL
Copy link
Copy Markdown
Contributor

LiquidPL commented Apr 3, 2026

I believe that my change in #37172 started causing some issues because suddenly the test client started getting updates that it previously weren't. oh yeah that's literally what the PR message mentions

Overall the tests are a mess since in many places it's using regular multiplayer room logic (eg. using CreateDefaultRoom which will insert a playlist item on room creation because that's what multiplayer rooms do that breaks assumptions of ranked play which expects no playlists items until a card is played.

@LiquidPL
Copy link
Copy Markdown
Contributor

LiquidPL commented Apr 3, 2026

I think this is fine for now, a lot of the ranked play tests will need to be rethought once we have time for that.

@bdach bdach mentioned this pull request Apr 3, 2026
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ranked-play area:test coverage Adds or modifies test coverage without adding any functionality. size/S

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants