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

Remove most usages of TestDispatcher in paging-common tests #523

Conversation

veyndan
Copy link
Contributor

@veyndan veyndan commented Apr 19, 2023

Removes all usages of TestDispatcher in the Paging 3 parts of paging-common tests, and some usages of TestDispatcher in the Paging 2 parts of paging-common tests (PagedListTest.kt and ContiguousPagedListTest.kt are left).

Test: ./gradlew test connectedCheck
Bug: 275416222

// REFRESH will auto run consecutively and we won't be able to assert them incrementally
loadDispatcher.queue.poll()?.run()
loadDispatcher.scheduler.advanceTimeBy(1001)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default loadDelay of TestPagingSource is 1000ms. We want to advance the first one only, so any value between [1001, 1999] would be valid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks!

@claraf3
Copy link
Member

claraf3 commented Apr 19, 2023

The presubmit fails on paging-compose is our issue (as with your other PRs). Don't worry about it.

@claraf3 claraf3 self-requested a review April 19, 2023 17:57
@veyndan veyndan force-pushed the 275416222/rm-TestDispatcher-usages branch from 8df39fb to df63e58 Compare April 20, 2023 21:45
@veyndan veyndan force-pushed the 275416222/rm-TestDispatcher-usages branch from df63e58 to 9b82469 Compare April 24, 2023 08:30
@@ -2047,7 2048,6 @@ class PagingDataDifferTest(
initialKey = initialKey,
pagingSourceFactory = {
TestPagingSource(
loadDelay = 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If our tests are relying on a specific loadDelay, might be more reliable to declare a specific loadDelay so its not flaky to default impl changes. Perhaps set delay to 1000 here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Updated PR to explicitly set loadDelay = 1000.

@veyndan veyndan deleted the 275416222/rm-TestDispatcher-usages branch April 26, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants