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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

veyndan
Copy link
Contributor

@veyndan veyndan commented Mar 17, 2023

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

@veyndan veyndan mentioned this pull request Mar 17, 2023
Closed
@claraf3
Copy link
Member

claraf3 commented Mar 22, 2023

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
Copy link
Contributor Author

veyndan commented Mar 22, 2023 •
edited

@claraf3 Rebased all three!

@claraf3
Copy link
Member

claraf3 commented Mar 23, 2023

sweet thanks!

@claraf3
Copy link
Member

claraf3 commented Mar 23, 2023

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
Copy link
Contributor Author

veyndan commented Mar 27, 2023

@claraf3 Yep, could do that instead.

Two things of note:

  1. 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.
  2. 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
Copy link
Member

claraf3 commented Mar 27, 2023 •
edited

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.

copybara-service bot pushed a commit that referenced this pull request Mar 29, 2023
@veyndan
[GH] Replace usage of DirectDispatcher with EmptyCoroutineContext in …
aa5145a 
…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
Copy link
Member

claraf3 commented Mar 30, 2023

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
Copy link
Contributor Author

veyndan commented Mar 30, 2023

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
Copy link
Member

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
Copy link
Member

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.

@veyndan
Copy link
Contributor Author

veyndan commented Apr 6, 2023

@ianhanniballake Done!

@copybara-service copybara-service bot closed this in 046cecf Apr 6, 2023
@veyndan veyndan deleted the 270612487/rm-dep branch April 6, 2023 21:07
@veyndan veyndan mentioned this pull request Apr 14, 2023
Closed
@veyndan
Copy link
Contributor Author

veyndan commented Apr 14, 2023

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

@claraf3
Copy link
Member

claraf3 commented Apr 14, 2023

Nice, thanks @veyndan!

copybara-service bot pushed a commit that referenced this pull request May 9, 2023
@veyndan @claraf3
[GH] Kmp-ify testutils-ktx
71a449b 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianhanniballake ianhanniballake ianhanniballake approved these changes

@claraf3 claraf3 claraf3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@veyndan @claraf3 @ianhanniballake