Make Song Select v2 the new default #33970
Conversation
This is being run in the flow where we are providing a specific beatmap for immediately selection. In an edge case scenario, the carousel may be pending on a filter operation, which would cause the whole `SelectAndRun` call to fail when it doesn't need to. This is reproduced by multiple test scenes. One example is `TestSceneOpenEditorTimestamp.TestErrorNotifications`.
peppy
force-pushed
the
song-select-v2-default
branch
from
a40b622 to
94d6260
Compare
| @@ -81,11 +81,9 @@ public void TestFromSongSelectDifferentRulesetWithConvertDisallowed() | |||
| presentAndConfirm(osuImport); | |||
|
|
|||
| var maniaImport = importBeatmap(2, new ManiaRuleset().RulesetInfo); | |||
| confirmBeatmapInSongSelect(maniaImport); | |||
There was a problem hiding this comment.
This was confirming panel presence, which is no longer a thing because panels which are filtered away do not get created upfront.
|
First run overlay has a weird issue, footer not displaying and logo in wrong place:
Fixed via 46dab76 |
|
|
||
| AddStep("Present same beatmap", () => Game.PresentBeatmap(Game.BeatmapManager.QueryBeatmapSet(set => set.ID == beatmapSetGuid)!.Value, beatmap => beatmap.ID == beatmapGuid)); | ||
| AddUntilStep("Wait for beatmap selected", () => Game.Beatmap.Value.BeatmapInfo.ID == beatmapGuid); | ||
| AddStep("Open options", () => InputManager.Key(Key.F3)); |
There was a problem hiding this comment.
Still have to fix these hotkeys no longer aligning. Took the easier way out for now though.
|
tests timed out... 🙈 |
|
Timeouts will be related to . I can repro locally but very rarely, will investigate. |
Disposal woes.
| if (!ensureGlobalBeatmapValid()) | ||
| return; |
There was a problem hiding this comment.
this removed call is referenced in a commit a few lines above that should probably be trimmed
osu/osu.Game/Screens/SelectV2/SongSelect.cs
Lines 421 to 424 in 579b547
There was a problem hiding this comment.
i'm choosing to trust that all relevant tests from here have already been ported for song select v2 and am not double checking it
There was a problem hiding this comment.
I did one pass but it wasn't exhaustive. I'll do a second pass tomorrow comparing test-for-test.
There was a problem hiding this comment.
This one is an absolute clusterfuck as expected. And I'm not sure what the forward path here is. Probably need to go through one-by-one on the removed methods and check whether we want to add back coverage?
Would you rather that's done in this PR or separately? My preference would be opening an issue tracking this separately so it's not blocking this already-delayed task of getting song select out on tachyon as default.
diff --git a/test_comparison b/test_comparison
index e66df27cc2..8c57a07f8f 100644
--- a/test_comparison
+++ b/test_comparison
@@ -1,46 +1,25 @@
-TestScenePlaySongSelect
+TestSceneSongSelect
-TestAudioRemainsCorrectOnRulesetChange(bool rulesetsInSameBeatmap)
-TestAudioResuming()
TestAutoplayShortcut()
TestAutoplayShortcutKeepsAutoplayIfSelectedAlready()
TestAutoplayShortcutReturnsInitialModsOnExit()
-TestBeatmapOptionsDisabled()
-TestCarouselSelectionUpdatesOnResume()
-TestChangeBeatmapAfterEnter()
-TestChangeBeatmapBeforeEnter()
-TestChangeBeatmapViaMouseAfterEnter()
-TestChangeBeatmapViaMouseBeforeEnter()
-TestChangeBeatmapWhilePresentingScore()
-TestChangeRulesetWhilePresentingScore()
-TestChangingRulesetOnMultiRulesetBeatmap()
-TestCutInFilterTextBox()
+TestClearModsViaModButtonRightClick()
+TestCookieDoesNothingIfNothingSelected()
TestDeleteHotkey()
-TestDifficultyIconSelecting()
-TestDifficultyIconSelectingForDifferentRuleset()
-TestDummy()
-TestExternalBeatmapChangeWhileFiltered(bool differentRuleset)
-TestExternalBeatmapChangeWhileFilteredThenRefilter()
-TestFilterOnResumeAfterChange()
-TestFilterableModChange()
-TestGroupedDifficultyIconSelecting()
-TestHardDeleteHandledCorrectly()
-TestHideSetSelectsCorrectBeatmap()
-TestImportUnderCurrentRuleset()
-TestImportUnderDifferentRuleset()
-TestModOverlayToggling()
-TestModsRetainedBetweenSongSelect()
-TestNoFilterOnSimpleResume()
-TestNonFilterableModChange()
-TestPlaceholderBeatmapPresence()
-TestPlaceholderConvertSetting()
-TestPlaceholderStarDifficulty()
-TestPresentNewBeatmapNewRuleset()
-TestPresentNewRulesetNewBeatmap()
-TestSearchTextWithRulesetCriteria()
-TestSelectionRetainedOnBeatmapUpdate()
-TestSingleFilterOnEnter()
-TestSorting()
+TestFooterModOverlay()
+TestFooterMods()
+TestFooterOptions()
+TestFooterOptionsState()
+TestFooterRandom()
+TestFooterRandomViaMouse()
+TestFooterRewind()
+TestFooterRewindViaMouseRight()
+TestFooterRewindViaShiftMouseLeft()
+TestInvalidRulesetDoesNotEnterGameplay()
+TestModSelectCannotBeOpenedAfterConfirmingSelection()
+TestResultsScreenWhenClickingLeaderboardScore()
+TestSelectAfterDeletion()
+TestSelectionChangedFromProtectedToNone()
+TestSelectionChangedFromProtectedToSomething()
+TestSingleFilterWhenEntering()
TestSpeedChange()
-TestStartAfterUnMatchingFilterDoesNotStart()
-TestTextBoxBeatmapDifficultyCount()
There was a problem hiding this comment.
I'm fine doing this separately as long as it's tracked.
There was a problem hiding this comment.
same here, trusting the new one has comparable coverage here
There was a problem hiding this comment.
This one has 100% coverage (one new test)
diff --git a/test_comparison b/test_comparison
index fc96594abc..205a2ba503 100644
--- a/test_comparison
+++ b/test_comparison
@@ -1,4 +1,4 @@
-TestSceneBeatmapLeaderboard
+TestSceneBeatmapLeaderboardWedge
TestPersonalBest()
TestGlobalScoresDisplay()
@@ -7,3 +7,4 @@ TestLocalScoresDisplayWorksWhenStartingOffline()
TestLocalScoresDisplayOnBeatmapEdit()
TestPlaceholderStates()
TestUseTheseModsDoesNotCopySystemMods()
+TestPersonalBestWithNullPosition()
There are still multiple issues with the new song select, but the most critical ones will be fixed before the next mainstream release. In the mean time, getting the new version in front of devs and (tachyon) users is a good next step.
I can imagine this is going to be a classified in the "painful review" category, but the majority of the changes are in test logic. I did have to refactor or rewrite multiple tests (and delete a few) in order to account for intentional behavioural differences.
The two non-test behavioural changes (562c637 and f8e8041) fix tests which failed after being migrated to v2, where the old behaviours were expected but not implemented yet in v2.