refactor: CardBrowserTest + fix flaky tests #12263
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
david-allison
added
Review High Priority
This comment was marked as resolved.
This comment was marked as resolved.
david-allison
added
Needs Author Reply
Before the introduction of `SingleTaskManager`, all `CollectionTask`s required `advanceRobolectricLooper()`` After `SingleTaskManager`, CollectionTask no longer required `advanceRobolectricLooper()` to execute. Sometimes `advanceRobolectricLooper` was required for non CollectionTask-based calls (for example: calling `onCreateOptionsMenu` at the correct time) We currently use `@RunInBackground` to use the old behaviour. This is not ideal as `advanceRobolectricLooperWithSleep` adds 500ms per call to the unit tests `advanceRobolectricUiLooper` method exists for the migration away from @RunInBackground, where calls are known to be necessary The overall aim is: * Remove `@RunInBackground` and replace with `advanceRobolectricUiLooper` calls when necessary * Remove ALL legacy calls to `advanceRobolectricLooperWithSleep` * Lots of these exist as a partial migration from `@RunInBackground` * Remove ALL legacy calls to `advanceRobolectricLooper`
introduce: `advanceRobolectricUiLooper` on - `selectMenuItem` - `selectOneOfManyCards` Remove all unnecessary calls to: - `advanceRobolectricLooperWithSleep`
- grantWritePermissions - revokeWritePermissions
Introduce `withNoWritePermission` so this error is no longer possible We have flaky tests on this class, and it likely is not an issue but it seems wrong, and the fix is trivial
david-allison
force-pushed
the
fix-CardBrowserTest
branch
from
b95456c
to
8838b3d
Compare
|
I'm going to run the Windows CI a few times to see if this is fixed by 8f0c0e2
|
```
CardBrowserTest > browserIsInMultiSelectModeWhenSelectingAll FAILED
java.lang.AssertionError:
Expected: <true>
but: was <false>
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
at com.ichi2.anki.CardBrowserTest.browserIsInMultiSelectModeWhenSelectingAll(CardBrowserTest.kt:116)
CardBrowserTest > moveToNonDynamicDeckWorks FAILED
java.lang.IllegalStateException: Attempted to check card at index 0. 0 cards available
at com.ichi2.anki.CardBrowser.checkCardsAtPositions(CardBrowser.kt:2533)
at com.ichi2.anki.CardBrowserTest.moveToNonDynamicDeckWorks(CardBrowserTest.kt:207)
```
david-allison
added
Review High Priority
david-allison
changed the title
mikehardy
approved these changes
Sep 1, 2022
github-actions
bot
removed
Needs Review
Review High Priority
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose / Description
I was investigating the flaky unit tests, and wanted to see what was going on
I could not reproduce the test failure, but a few frustrations occurred:
Robolectric.startActivitywas introduced@KotlinCleanups@RunInBackgroundwas used, which is pending for removalrevokeWritePermissionsnot being reset, which may have caused test failuresI fixed all of these, and introduced
advanceRobolectricUiLooperto allow for further migration away from@RunInBackgroundI then added another
advanceRobolectricUiLooperafter creating theActivity, and the problems went away#11091
How Has This Been Tested?
Test only - tests run about 2 seconds faster expected - we shave 4*500ms off tests
Learning
advanceRobolectricUiLooperis still required -onCreateOptionsMenuwas not called otherwise due to an updateChecklist