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 more test failures #28900

Merged
merged 8 commits into from
Jul 19, 2024
Merged

Fix more test failures #28900

merged 8 commits into from
Jul 19, 2024

Conversation

smoogipoo
Copy link
Contributor

I'm going to open this as a WIP PR for easier tracking. It'll be finished when I'm happier with the test results.

https://github.com/ppy/osu/actions/runs/9985890747/job/27597501295

In this case, the settings overlay is taking a very long time to load
(on a background thread), and pops in when it finishes loading because
it's been requested to open.

The opens the settings overlay, closes it (by pressing escape, this does
not actually close it because it's not loaded yet), and then enters song
select by pressing 'P' 3 times. The settings overlay finishes loading at
just the right opportune moment to eat one of the 'P' key presses.
https://github.com/smoogipoo/osu/actions/runs/9986761756/job/27599851263

This is a bit of a workaround, likely timing related. I don't foresee an
until step in this case to cause false-passes.
https://github.com/smoogipoo/osu/actions/runs/9990112749/job/27610257309

Comments are loaded asynchronously, both from the initial request and
the following message-post request. By sheer timing luck, these could be
out of order and the assertion on the posted message could fail.
@smoogipoo
Copy link
Contributor Author

smoogipoo commented Jul 18, 2024

Re: 3f4e56b

The test failure here can be reproed 100% on both macOS-arm64 and linux-x64 (on my beefy PC) under smoogipoo@a522c0e

It's very weird so I'm investigating further. It's like our ThreadedTaskScheduler is just not running fast enough/at all. I expect the BDL threads to easily complete fast enough.

We have seen it on master: https://github.com/ppy/osu/actions/runs/9981737481/job/27587894137

It could be something super dumb, like JIT startup time.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 19, 2024
https://github.com/ppy/osu/pull/28900/checks?check_run_id=27652166871

This is an attempt. Going frame-by-frame I noticed that there's one
frame in which the text is loaded but the
FillFlowContainer/GridContainer haven't properly validated so the text
is not positioned correctly (it's overflowing the panel to the left). If
the cursor is moved at this exact time, then it may not be properly
positioned for the following assertion, even though it is _somewhere_ on
the panel.

If the above is the case, then this is a known o!f issue, but not a
simple one to solve.

I haven't reproed this locally.
Makes it easy to compare this line versus the one in
OsuGame.PresentBeatmap(). At the moment it's just GUID which is... not
useful!
@smoogipoo smoogipoo marked this pull request as ready for review July 19, 2024 10:07
@smoogipoo
Copy link
Contributor Author

I'm going to bring this out of draft now with a few fixes, going into the weekend. It's still not where I want it to be but the remaining test failures are weird - some are timing out because the BDL threads are not running fast enough.

@peppy peppy self-requested a review July 19, 2024 10:38
@peppy peppy merged commit d929743 into ppy:master Jul 19, 2024
9 of 11 checks passed
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.

2 participants