Use Duration in runTest builders as timeout by default #3483
Conversation
| @@ -22,7 +22,10 @@ public final class kotlinx/coroutines/test/TestBuildersKt { | |||
| public static final fun runTest (Lkotlinx/coroutines/test/TestScope;JLkotlin/jvm/functions/Function2;)V | |||
| public static synthetic fun runTest$default (Lkotlin/coroutines/CoroutineContext;JLkotlin/jvm/functions/Function2;ILjava/lang/Object;)V | |||
| public static synthetic fun runTest$default (Lkotlinx/coroutines/test/TestCoroutineScope;JLkotlin/jvm/functions/Function2;ILjava/lang/Object;)V | |||
| public static synthetic fun runTest$default (Lkotlinx/coroutines/test/TestScope;JLkotlin/jvm/functions/Function2;ILjava/lang/Object;)V | |||
There was a problem hiding this comment.
It's unpleasant that this overload has disappeared from the resulting binary. What I think we should do instead is to make the Duration overload not have a default argument, and change the Long overload back so that it has it. From the programmer's perspective, it doesn't matter, as the behavior stays exactly the same, but preserving compatibility is nice.
There was a problem hiding this comment.
Added, although it is experimental api.
But is there a bug in bcv? I removed the default value from runTest(Long), but there is no synthetic runTest$default function without TestScope receiver.
There was a problem hiding this comment.
Well, the source compatibility did break at least, causing the tests to fail.
There was a problem hiding this comment.
Which tests? I still guess, there is some bug in bcv (or I don't get binary compatibility).
Before:
runTest(long: default)TestScope.runTest(long: default)
Now:
runTest(duration: default)runTest(long: no default)<- I removed this default, but there is no removed line in.apiTestScope.runTest(duration: no default)TestScope.runTest(long: default)<- added back the default
There was a problem hiding this comment.
Which tests?
The tests in general didn't even compile for me, they were unable to find a runTest overload that could be invoked just as runTest { ... }.
I removed this default, but there is no removed line in
.api
It's my understanding that having at least one optional parameter creates a single $default overload that allows passing all the parameters in the Object. So, since the context parameter still had a default value, the overload was created, and the binary compatibility was preserved. @qwwdfsad, could you confirm or deny this?
There was a problem hiding this comment.
Kind of, except that the trailing Object parameter is never used, it's just a rudiment that we are unlikely to ever use.
Duration is (finally) stable and its usage is easier to read. This change only affects the runTest builder. You could still use a Long as a timeout, but the default now uses Duration. I didn't add new tests, because
runTestwithout a custom timeout is used a lot, and usesDurationnow. Also, the timeout operator is tested with Duration too. But I am open to adding tests :)#3135 (comment)