Replace usage of TestDispatcher with StandardTestDispatcher in paging-compose by veyndan · Pull Request #505 · androidx/androidx

veyndan · 2023-03-17T17:31:51Z

In Multiplatform Paging, the classes DirectDispatcher and TestDispatcher in testutils-ktx had to be multiplatformized to allow dependent tests in paging-compose, paging-common, and paging-runtime to be moved to the respective commonTest folders.

I'm attempting to replace all usages of DirectDispatcher and TestDispatcher that Multiplatform Paging depends on with those found in kotlinx.coroutines.test, so hopefully the testutils-ktx dependency no longer has to be multiplatformized in my fork.

I'm starting this effort by doing it with paging-compose.

Test: ./gradlew test connectedCheck
Bug: 270612487

claraf3 · 2023-03-22T21:02:31Z

Hey thanks for the PR. Sorry took me to some time to get to it. I think this and PR 506, 507 are failing from a metalava error that has since been fixed. Can you try rebasing?

veyndan · 2023-03-22T21:20:17Z

@claraf3 Rebased all three!

claraf3 · 2023-03-23T00:33:04Z

sweet thanks!

claraf3 · 2023-03-23T22:08:45Z

Im thinking we can kmp-ify testutils-ktx directly? If you already have a local fork of it, can you upload a PR of it?

veyndan · 2023-03-27T17:18:33Z

@claraf3 Yep, could do that instead.

Two things of note:

I went down the route of replacing TestDispatcher with StandardTestDispatcher, because personally I found it easier to read as I'm already aware of StandardTestDispatcher because it's in kotlin-test, whereas TestDispatcher is specific to androidx.
My fork only multiplatformizes a couple of classes, not all of them.

I don't feel strongly about this, so if you think it makes sense to keep using TestDispatcher, I can close out this PR and submit a PR that multiplatformizes a couple of classes from testutils-ktx.

claraf3 · 2023-03-27T21:37:54Z

Ideally our tests would rely on standard kotlin-test classes but it that would touch a lot of our tests so I'll have to take a closer look at that at a later time. But in the meantime I think it makes sense to upstream your work here. Even if we replace use of custom dipatchers, its probably better in the long run to have testutil-ktx multiplatform-ready.

Would appreciate it if you close out these few PRs and we have a PR with your multiplatformized classes from testutil-ktx as you suggested. Thank you.

…testutils-paging The reasoning is the same as #505, but for `testutils-paging`. Test: ./gradlew test connectedCheck Bug: 270612487 This is an imported pull request from #507. Resolves #507 Github-Pr-Head-Sha: d1497e7 GitOrigin-RevId: e1f3a08 Change-Id: Idcd1a1981941baf4ddd93346f87b9e30dbcb746e

claraf3 · 2023-03-30T17:21:56Z

Hi @veyndan, we merged the other two PRs to start moving off of testutils-ktx. We can also merge your multiplatformized classes from testutil-ktx if you're still interested in doing that.

veyndan · 2023-03-30T19:07:18Z

Yep I am — just need to find some time to do it! I'd probably create a separate PR though.

As one of the PRs that were merged was very similar to this one (#507), would it make sense to upstream this one too? It isn't necessary as part of maintaining Multiplatform Paging once I've created the other PR, but just if there's a desire to start using classes from kotlin-test instead of testutils-ktx, then this PR contributes to that effort.

ianhanniballake · 2023-03-30T19:36:28Z

Yep, I think we'd like to move to kotlin-test wherever possible and only rely on testutils-ktx if we have a specific need that goes beyond what kotlin-test provides.

Although ideally we would rewrite our tests so that we can always rely on kotlin-test (or Turbine, to be honest) rather than maintaining our own custom coroutine test utils, but obviously that is a larger migration.

ianhanniballake · 2023-03-31T20:08:00Z

We're seeing some errors in CI that looks like a missing @ExperimentalCoroutinesApi:

build_log_simplifier.py: Error: Found 11 new lines of warning output!

The new output:
  > Task :paging:paging-compose:compileDebugAndroidTestKotlin
  w: file://$SUPPORT/paging/paging-compose/src/androidTest/java/androidx/paging/compose/LazyPagingItemsTest.kt:824:23 This declaration needs opt-in. Its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
  w: file://$SUPPORT/paging/paging-compose/src/androidTest/java/androidx/paging/compose/LazyPagingItemsTest.kt:827:67 This declaration needs opt-in. Its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
  w: file://$SUPPORT/paging/paging-compose/src/androidTest/java/androidx/paging/compose/LazyPagingItemsTest.kt:831:25 This declaration needs opt-in. Its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
  w: file://$SUPPORT/paging/paging-compose/src/androidTest/java/androidx/paging/compose/LazyPagingItemsTest.kt:837:9 This declaration needs opt-in. Its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
  w: file://$SUPPORT/paging/paging-compose/src/androidTest/java/androidx/paging/compose/LazyPagingItemsTest.kt:837:17 This declaration needs opt-in. Its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
  w: file://$SUPPORT/paging/paging-compose/src/androidTest/java/androidx/paging/compose/LazyPagingItemsTest.kt:837:27 This declaration needs opt-in. Its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
  w: file://$SUPPORT/paging/paging-compose/src/androidTest/java/androidx/paging/compose/LazyPagingItemsTest.kt:841:13 This declaration needs opt-in. Its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
  w: file://$SUPPORT/paging/paging-compose/src/androidTest/java/androidx/paging/compose/LazyPagingItemsTest.kt:841:21 This declaration needs opt-in. Its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
  w: file://$SUPPORT/paging/paging-compose/src/androidTest/java/androidx/paging/compose/LazyPagingItemsTest.kt:841:31 This declaration needs opt-in. Its usage should be marked with '@kotlinx.coroutines.ExperimentalCoroutinesApi' or '@OptIn(kotlinx.coroutines.ExperimentalCoroutinesApi::class)'
  


To reproduce this failure:
  Try $ ./gradlew -Pandroidx.validateNoUnrecognizedMessages --rerun-tasks :paging:paging-compose:compileDebugAndroidTestKotlin

Can you please add the correct annotations? That last line should help verify that you've caught everything.

…-compose

veyndan · 2023-04-06T14:26:56Z

@ianhanniballake Done!

veyndan · 2023-04-14T13:55:52Z

@claraf3 Here's the PR to kmp-ify testutils-ktx!

claraf3 · 2023-04-14T17:57:43Z

Nice, thanks @veyndan!

Upstreaming [Multiplatform Paging's kmp-ified testutils-ktx](https://github.com/cashapp/multiplatform-paging/tree/androidx-main-3.2.0-alpha04/testutils/testutils-ktx). See #505 (comment) for more context. Test: ./gradlew test connectedCheck Bug: 270612487 This is an imported pull request from #518. Resolves #518 Github-Pr-Head-Sha: a261291 GitOrigin-RevId: 4d77f0b Change-Id: I2c3e46143ffa369aafed9bbc9da05d4b4636e6d0

…testutils-paging The reasoning is the same as androidx#505, but for `testutils-paging`. Test: ./gradlew test connectedCheck Bug: 270612487 This is an imported pull request from androidx#507. Resolves JetBrains#507 Github-Pr-Head-Sha: d1497e7 GitOrigin-RevId: e1f3a08 Change-Id: Idcd1a1981941baf4ddd93346f87b9e30dbcb746e

…aging-compose In Multiplatform Paging, the classes `DirectDispatcher` and `TestDispatcher` in `testutils-ktx` had to be multiplatformized to allow dependent tests in `paging-compose`, `paging-common`, and `paging-runtime` to be moved to the respective `commonTest` folders. I'm attempting to replace all usages of `DirectDispatcher` and `TestDispatcher` that Multiplatform Paging depends on with those found in `kotlinx.coroutines.test`, so hopefully the `testutils-ktx` dependency no longer has to be multiplatformized in my fork. I'm starting this effort by doing it with `paging-compose`. Test: ./gradlew test connectedCheck Bug: 270612487 This is an imported pull request from androidx#505. Resolves JetBrains#505 Github-Pr-Head-Sha: 002b706 GitOrigin-RevId: 2b32273 Change-Id: Id018cf4bf64dac3154eff2ea98e6145aff4290fc

Upstreaming [Multiplatform Paging's kmp-ified testutils-ktx](https://github.com/cashapp/multiplatform-paging/tree/androidx-main-3.2.0-alpha04/testutils/testutils-ktx). See androidx#505 (comment) for more context. Test: ./gradlew test connectedCheck Bug: 270612487 This is an imported pull request from androidx#518. Resolves JetBrains#518 Github-Pr-Head-Sha: a261291 GitOrigin-RevId: 4d77f0b Change-Id: I2c3e46143ffa369aafed9bbc9da05d4b4636e6d0

veyndan mentioned this pull request Mar 17, 2023

Replace usage of DirectDispatcher with EmptyCoroutineContext in testutils-paging #507

Closed

claraf3 requested review from claraf3 and ianhanniballake March 22, 2023 20:19

veyndan force-pushed the 270612487/rm-dep branch from 12abfc2 to 2cd7d4e Compare March 22, 2023 21:18

claraf3 approved these changes Mar 23, 2023

View reviewed changes

ianhanniballake approved these changes Mar 31, 2023

View reviewed changes

veyndan added 2 commits April 6, 2023 12:24

Replace usage of TestDispatcher with StandardTestDispatcher in paging…

3b9cbfe

…-compose

Add missing OptIn annotation

b44c4f7

veyndan force-pushed the 270612487/rm-dep branch from 002b706 to b44c4f7 Compare April 6, 2023 10:25

copybara-service Bot closed this in 046cecf Apr 6, 2023

veyndan deleted the 270612487/rm-dep branch April 6, 2023 21:07

veyndan mentioned this pull request Apr 14, 2023

Kmp-ify testutils-ktx #518

Closed

Navigation Menu

Uh oh!

Replace usage of TestDispatcher with StandardTestDispatcher in paging-compose - #505

Replace usage of TestDispatcher with StandardTestDispatcher in paging-compose#505
veyndan wants to merge 2 commits into
androidx:androidx-mainfrom
veyndan:270612487/rm-dep

veyndan commented Mar 17, 2023

Uh oh!

claraf3 commented Mar 22, 2023

Uh oh!

veyndan commented Mar 22, 2023 •

edited

Loading

Uh oh!

claraf3 commented Mar 23, 2023

Uh oh!

claraf3 commented Mar 23, 2023

Uh oh!

veyndan commented Mar 27, 2023

Uh oh!

claraf3 commented Mar 27, 2023 •

edited

Loading

Uh oh!

claraf3 commented Mar 30, 2023

Uh oh!

veyndan commented Mar 30, 2023

Uh oh!

ianhanniballake commented Mar 30, 2023

Uh oh!

ianhanniballake commented Mar 31, 2023

Uh oh!

veyndan commented Apr 6, 2023

Uh oh!

veyndan commented Apr 14, 2023

Uh oh!

claraf3 commented Apr 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

veyndan commented Mar 17, 2023

Uh oh!

claraf3 commented Mar 22, 2023

Uh oh!

veyndan commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claraf3 commented Mar 23, 2023

Uh oh!

claraf3 commented Mar 23, 2023

Uh oh!

veyndan commented Mar 27, 2023

Uh oh!

claraf3 commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claraf3 commented Mar 30, 2023

Uh oh!

veyndan commented Mar 30, 2023

Uh oh!

ianhanniballake commented Mar 30, 2023

Uh oh!

ianhanniballake commented Mar 31, 2023

Uh oh!

veyndan commented Apr 6, 2023

Uh oh!

veyndan commented Apr 14, 2023

Uh oh!

claraf3 commented Apr 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

veyndan commented Mar 22, 2023 •

edited

Loading

claraf3 commented Mar 27, 2023 •

edited

Loading