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

Implement 'runTest' that waits for asynchronous completion #2978

Conversation

dkhalanskyjb
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb commented Oct 12, 2021

Implement a multiplatform runTest as an initial implementation of #1996.

The new runTest also waits for asynchronous completion, which makes this a fix for a long-standing issue.
Fixes #1204
Fixes #1222
Fixes #1395
Fixes #1881
Fixes #1910
Fixes #1365 (though there may be some other use cases that this solution doesn't cover)
Fixes #1772

@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-coroutineScope branch 2 times, most recently from 3e872b4 to f90ba4c Compare October 13, 2021 15:01
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-await-other-dispatchers branch 5 times, most recently from e2bbbf6 to 19ed1f0 Compare October 14, 2021 19:19
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-await-other-dispatchers branch 2 times, most recently from 98102de to 608783e Compare October 15, 2021 12:33
@dkhalanskyjb dkhalanskyjb marked this pull request as ready for review October 15, 2021 12:33
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-await-other-dispatchers branch from 608783e to d694912 Compare October 25, 2021 12:34
kotlinx-coroutines-test/common/src/TestBuilders.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-test/common/src/TestBuilders.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-test/common/src/TestBuilders.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-test/common/src/TestBuilders.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-test/common/test/RunTestTest.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-test/common/src/TestBuilders.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-test/common/src/TestBuilders.kt Outdated Show resolved Hide resolved
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-await-other-dispatchers branch from d694912 to 77d524f Compare October 26, 2021 07:29
* Add more detailed documentation;
* Move most verification logic from `runBlockingTest` to
  `cleanupTestCoroutines`
Fixes #1749
* Complete the scope's job if a new job was created for it
Fixes #1772
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-await-other-dispatchers branch from 77d524f to 78e5855 Compare October 27, 2021 08:07
@dkhalanskyjb dkhalanskyjb force-pushed the coroutines-test-await-other-dispatchers branch from 15d1dec to a8e43d6 Compare October 28, 2021 11:41
Base automatically changed from coroutines-test-coroutineScope to coroutines-test-virtualtime October 28, 2021 12:10
kotlinx-coroutines-test/common/src/TestBuilders.kt Outdated Show resolved Hide resolved
* immediately from the test body. See the docs for [TestResult] for details.
*/
@ExperimentalCoroutinesApi
public fun TestDispatcher.runTest(
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this one if #3006 gets merged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It has its uses. People also create dispatchers in advance to perform DI.

In any case, this method looks fairly benign and doesn't hurt anything, so a more important question is whether we need TestCoroutineScope.runTest, which presupposes that we provide ways to create TestCoroutineScope. If we don't, then this convenience method can also go.

Copy link
Member

Choose a reason for hiding this comment

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

Let's make this decision in the final branch along with the migration guide.

I still believe TestCoroutineScope.runTest may be useful for the examples like this one:

@Before 
fun before() {
    myScope = TestCoroutineScope(...)
    myEntity = MyEntity(..., myScope)
}

@Test
fun scenario1() = myScope.runTest {
    myEntity.foo()
    testScheduler.advanceUntilIdle()
    verifyFoo()
}

@Test
fun scenario2() = myScope.runTest {
    myEntity.bar()
    testScheduler.advanceUntilIdle()
    verifyBar()
}

Maybe TestDispatcher.runTest has the same uses, I was thinking of removing it for the sake of keeping API compact -- it's easily emulated with runTest(dispatcher) and only provides one more method to be aware of/to study.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should note that the scope inside myScope.runTest { ... } is not the same thing as myScope, which may reduce the usefulness of the pattern.

Also, this code is missing a

@AfterTest
fun after() {
  myScope.cleanupTestCoroutines()
}

I think this is a major point. According to the comments at the bottom of https://youtrack.jetbrains.com/issue/KT-19813, AfterTest and BeforeTest don't properly allow returning a Promise, which means that either

  • the cleanup procedure also can't return a TestResult, or
  • we shouldn't promote cleaning up in AfterTest, but then we'll also have to avoid promoting initializing scopes in BeforeTest for symmetry.

* The platform difference entails that, in order to use this function correctly in common code, one must always
* immediately return the produced [TestResult] from the test method, without doing anything else afterwards. See
* [TestResult] for details on this.
*
Copy link
Member

Choose a reason for hiding this comment

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

Mentioning the scope/dispatcher and the ability to wait until all tasks are executed is valuable for this API and worth mentioning:

@Test
fun exampleTest() = runTest {
    val myComponent = MyComponent(this) // TODO: can't extract dispatcher here
    myComponent.executeAsyncOp()
    testScheduler.advanceUntilIdle()
    verifyAsyncOpCompleted()
}

I do not insist though, but it seems pretty important, WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, added a mention of this.

@dkhalanskyjb dkhalanskyjb merged commit 9fb2182 into coroutines-test-virtualtime Nov 1, 2021
@dkhalanskyjb dkhalanskyjb deleted the coroutines-test-await-other-dispatchers branch November 1, 2021 13:26
dkhalanskyjb added a commit that referenced this pull request Nov 17, 2021
Implement a multiplatform runTest as an initial implementation of #1996.

Fixes #1204
Fixes #1222
Fixes #1395
Fixes #1881
Fixes #1910
Fixes #1772
dkhalanskyjb added a commit that referenced this pull request Nov 17, 2021
Implement a multiplatform runTest as an initial implementation of #1996.

Fixes #1204
Fixes #1222
Fixes #1395
Fixes #1881
Fixes #1910
Fixes #1772
dkhalanskyjb added a commit that referenced this pull request Nov 19, 2021
Implement a multiplatform runTest as an initial implementation of #1996.

Fixes #1204
Fixes #1222
Fixes #1395
Fixes #1881
Fixes #1910
Fixes #1772
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.

None yet

2 participants