Skip to content

skip delays #3094

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

Closed
caffeine-mgn opened this issue Dec 17, 2021 · 8 comments
Closed

skip delays #3094

caffeine-mgn opened this issue Dec 17, 2021 · 8 comments

Comments

@caffeine-mgn
Copy link

How I can disable delays skiping in tests?
I trying to write network library for kotlin-common (https://github.com/caffeine-mgn/pw.binom.io/tree/master/network). And I shoud real wait until client connect to server. Network work in other thread.

@caffeine-mgn
Copy link
Author

Also, the motivation for skipping delays is not clear. If the delay is too long or will create new threads, then these are the problems of those who use it

@dkhalanskyjb
Copy link
Collaborator

Hi!

In fact, the main point of the test framework is to skip delays. The motivation for this is simple: it is done so that tests don't take too long to complete, but instead finish immediately. The expectation is that every component of the system is mocked for testing. For example, it's common to avoid connecting to a real server from tests but instead, use a fake "server" returning pre-defined responses.

Delays are skipped only in the TestDispatcher. Use withContext(Dispatchers.Default) { ... }, for example, or, really, any other dispatcher, and the delays won't be skipped.

This is also described in the documentation, take a look if you're interested: https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-test/README.md Also, please use the public Slack channel or Stack Overflow for such general questions.

@joffrey-bion
Copy link
Contributor

joffrey-bion commented Dec 17, 2021

In fact, the main point of the test framework is to skip delays.

@dkhalanskyjb I think this was true when the kotlinx-coroutines-test library was JVM-only, because people could just use runBlocking otherwise. But now that it's multiplatform, I believe its primary purpose is to test code using coroutines, and in particular provide a way to run suspend functions in tests on JS, and on non-trivial native platforms like iOS where runBlocking doesn't fit well. Skipping delays is a nice added benefit, but not absolutely necessary nor always desirable.

For me the main reason for the switch is to get rid of my poor custom implementation for iOS suspending tests:
https://github.com/joffrey-bion/krossbow/blob/ef393fd340c06578387dbe1a55ff6e83d04da9d2/krossbow-websocket-test/src/iosMain/kotlin/org/hildan/krossbow/websocket/test/TestUtilsIos.kt

Parts of my tests for this websocket library are using an Autobahn Test Suite server (which is a fast but real server in another process), and I'm seeing instant timeout failures there because of the delay skipping. This could be a problem for any sort of multiplatform integration test using coroutines.

I have quickly tried the mentioned workaround for JVM and JS tests and indeed wrapping all such tests this way seems to work:

@Test
fun test() = runTest {
    withContext(Dispatchers.Default) {
        TODO("test code here")
    }
}

But I don't believe this will work in iOS tests - I had to use the main thread and go through all this trouble before. Also it means I'm using a multithreaded dispatcher on JVM, so I'll need to put more thought into it.

Overall, I think it would be nice if this was supported by default by the library so we wouldn't have to still write multiplatform helpers, that are probably poorly implemented. For instance runTest could have a skipDelays parameter that would be true by default, but could be set to false if the need arises.

...or we could have a runTestRespectingDelays (that's mostly a joke about our discussion for the runTest name - please don't take it too seriously)

@caffeine-mgn
Copy link
Author

@joffrey-bion I implement WS client and server using sockets for k/n (native, jvm) :)
https://github.com/caffeine-mgn/pw.binom.io/blob/master/httpClient/src/commonMain/kotlin/pw/binom/io/httpClient/HttpRequest.kt#L41

@dkhalanskyjb Strange decision. Long test execution is a problem for those who write them. It seems to me that your solution might help in extreme rare cases. For example, a delay before the response of a function ... but why is it?
I think that if there is a delay, then it is not casual.

@joffrey-bion
Copy link
Contributor

joffrey-bion commented Dec 17, 2021

It seems to me that your solution might help in extreme rare cases. For example, a delay before the response of a function ... but why is it? I think that if there is a delay, then it is not casual.

@caffeine-mgn I disagree here. Most delays that are used for schedulers and timeouts of many sorts are very nicely testable with the controlled time approach given by runTest - I use this extensively, not necessarily by "skipping delays" per se, but mostly by manually controlling time, and this is very very useful (advanceTimeBy, advanceUntilIdle etc.).

The only problem I have with it is what I mentioned above: when you need to interact with something that is not time-controlled during your test, the delay skipping just breaks the behaviour.

@dkhalanskyjb
Copy link
Collaborator

on non-trivial native platforms like iOS where runBlocking doesn't fit well

Could you elaborate on this? runBlocking is, in fact, used internally for runTest on the native platforms.

Also it means I'm using a multithreaded dispatcher on JVM, so I'll need to put more thought into it.

If you don't want to use a multithreaded dispatcher, you can choose not to:

@Test
fun test() = runTest {
    withContext(Dispatchers.Default.limitedParallelism(1)) {
        TODO("the test code here will run in a single thread")
    }
}

Any dispatcher not linked to a TestCoroutineScheduler will not skip delays, including single-threaded ones.

But I don't believe this will work in iOS tests - I had to use the main thread and go through all this trouble before.

If you don't mock Dispatchers.Main with Dispatchers.setMain and use Dispatchers.Main in a test, it won't be a test dispatcher, so won't skip delays.

For instance runTest could have a skipDelays parameter that would be true by default, but could be set to false if the need arises.

I think such a parameter would only be very slightly less cumbersome than forking off to a different dispatcher that doesn't know about the virtual time, and is much less flexible: one could conceivably only need a part of the test to respect the delays.

@joffrey-bion
Copy link
Contributor

joffrey-bion commented Dec 17, 2021

on non-trivial native platforms like iOS where runBlocking doesn't fit well

Could you elaborate on this?

I would have to check how this works with the new memory model (I don't have a mac, so testing those iOS tests has a very slow feedback loop for me using the CI), but essentially the issue boiled down to the combination of 2 things:

  1. iOS test entrypoint calls the test methods on the main thread, and the test is run synchronously so I need to block the main thread while running the test code (this is painfully obvious when using runBlocking, but any other contrived solution has to somehow block the main thread in any case so the test framework knows when the test ends)
  2. iOS functions like NSURLSessionWebSocketTask.receiveMessageWithCompletionHandler needed to be run in the current (main) queue (at least with the old memory model), so the main thread needs to be free in order for the callback to be called - something needs to consume the main thread's event loop

So I initially wrote my test util using runBlocking but launching the test body in a separate coroutine, and concurrently with that I would manually consume the main event loop from the body of runBlocking:
joffrey-bion/krossbow@68d7e27

Which in the end I simplified this way:
https://github.com/joffrey-bion/krossbow/blob/ef393fd340c06578387dbe1a55ff6e83d04da9d2/krossbow-websocket-test/src/iosMain/kotlin/org/hildan/krossbow/websocket/test/TestUtilsIos.kt

But for some reason with coroutines 1.6.0 (non -native-mt) this now fails with:

kotlin.IllegalStateException: There is no event loop. Use runBlocking { ... } to start one.

Which I'm sure I had encountered when doing trial and error on this iOS test util, but this last approach worked fine in 1.5.2-native-mt. Maybe I'll need to go back to wrapping it in runBlocking in addition to consuming the main loop. But I'm hoping that runTest will solve this 🤞, so I can just remove all my custom code.

runBlocking is, in fact, used internally for runTest on the native platforms

Then you probably are consuming the main loop as well, concurrently with the test body, right?

I think such a parameter would only be very slightly less cumbersome than forking off to a different dispatcher that doesn't know about the virtual time

When writing common code, if something needs me to write platform specific code with expect-actual just for this withContext in all tests (and different dispatchers for different platforms), I find it much more cumbersome, not just slightly. Sometimes I wouldn't even need platform-specific source folders at all if it weren't for this.

This is why I was waiting for kotlinx-coroutines-test to become multiplatform so much, because it removes a LOT of custom expect/actual code. It wasn't for the delay skipping feature

@joffrey-bion
Copy link
Contributor

Here is another example of someone needing to run real-time things in their tests, and failing to do so using runTest:
https://stackoverflow.com/questions/70658926/how-to-use-kotlinx-coroutines-withtimeout-in-kotlinx-coroutines-test-runtest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants