-
Notifications
You must be signed in to change notification settings - Fork 630
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
Refactor concurrency #3623
Refactor concurrency #3623
Conversation
* Avoid the use of `runBlocking` inside coroutines. Schedule all coroutines below the test engine layer via regular dispatchers, bringing the test environment more in line with actual production environments. * Remove direct use of Java's `Executors`, delegating to kotlinx.coroutines-provided dispatchers instead. * Preserve coroutine context (which includes lots of Kotest configuration) in multithreaded test executions. * Remove hard-coded 1-day time limit for multithreaded test executions. * Change tests relying on single-threading from a coroutine model to a thread model, removing coroutine invocations (`delay`), replacing those with thread invocations (`Thread.sleep`). * Avoid leaking threads by closing dispatchers after use. * Keep the `assertionCounter` synchronized with thread-switching coroutines. * Remove `fun <K, V> concurrentHashMap(): MutableMap<K, V>`, which was not thread-safe. * Make `FixedThreadCoroutineDispatcherFactory.dispatcherAffinity` thread-safe. * Revert a change in `ConcurrentTestSuiteScheduler.schedule` by commit c316bbd, which replaced `launch` with `async` plus `joinAll`. This was functionally equivalent, but `async` was not needed, as there were no values returned, and `joinAll` was already performed by the enclosing `coroutineScope`.
# Conflicts: # kotest-extensions/kotest-extensions-blockhound/src/jvmTest/kotlin/io/kotest/extensions/blockhound/BlockHoundTest.kt
Test failure seems unrelated to this PR:
|
Prob some very intermittent prop test failure
…On Sun, Aug 27, 2023, 11:18 AM OliverO2 ***@***.***> wrote:
Test failure seems unrelated to this PR:
> Task :kotest-property:jvmTest
com.sksamuel.kotest.property.arbitrary.BindShrinkTest[jvm]
Test Shrinks all components to minimum value[jvm] FAILED
java.lang.AssertionError: "Property test failed for inputs
0) MaximumComponents(a=1000, b=1000, c=1000, d=1000, e=1000, f=1000, g=1000, h=1000, i=1000, j=1000, k=1000, l=1000, m=1000, n=1000)
Caused by java.lang.AssertionError: 1000 should be < 100 at
com.sksamuel.kotest.property.arbitrary.BindShrinkTest$1$2$stdout$1$1$1.invokeSuspend(BindShrinkTest.kt:52)
com.sksamuel.kotest.property.arbitrary.BindShrinkTest$1$2$stdout$1$1$1.invoke(BindShrinkTest.kt)
com.sksamuel.kotest.property.arbitrary.BindShrinkTest$1$2$stdout$1$1$1.invoke(BindShrinkTest.kt)
io.kotest.property.internal.ProptestKt$proptest$3$2.invokeSuspend(proptest.kt:49)
" should include substring "Shrink result (after 45 shrinks) => MaximumComponents(a=0, b=0, c=0, d=0, e=0, f=0, g=0, h=0, i=0, j=0, k=0, l=0, m=100, n=0)"
—
Reply to this email directly, view it on GitHub
<#3623 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFVSGXNZQ5OJ7TKCJHLDJ3XXNXMZANCNFSM6AAAAAA3UH74XE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Seems like this one is causing the test failure: inline fun captureStandardOut(fn: () -> Unit): String {
val previous = System.out
val buffer = ByteArrayOutputStream()
System.setOut(PrintStream(buffer))
try {
fn()
return String(buffer.toByteArray())
} finally {
System.setOut(previous)
}
}
|
This is green and ready to merge but I'd like to take a few days to read through and understand the changes if that's ok. |
That sounds fantastic! |
...ssertions/kotest-assertions-shared/src/jvmMain/kotlin/io/kotest/assertions/ErrorCollector.kt
Show resolved
Hide resolved
...t-framework-concurrency/src/jvmTest/kotlin/io/kotest/framework/concurrency/EventuallySpec.kt
Show resolved
Hide resolved
...test-framework-engine/src/commonMain/kotlin/io/kotest/engine/ConcurrentTestSuiteScheduler.kt
Show resolved
Hide resolved
...ework-engine/src/commonMain/kotlin/io/kotest/engine/listener/CollectingTestEngineListener.kt
Show resolved
Hide resolved
...ine/src/jvmMain/kotlin/io/kotest/engine/concurrency/FixedThreadCoroutineDispatcherFactory.kt
Show resolved
Hide resolved
...ework-engine/src/jvmTest/kotlin/com/sksamuel/kotest/engine/coroutines/provokeThreadSwitch.kt
Outdated
Show resolved
Hide resolved
...jvmTest/kotlin/com/sksamuel/kotest/engine/threads/BeforeTestConcurrentInstancePerLeafTest.kt
Show resolved
Hide resolved
Increase the minimum delay to 50 milliseconds based on observed behavior.
Couple more follow up questions added then we can merge in time for 5.7 release. |
nice work :) |
Thanks for the excellent cooperation. Way quicker than I had envisioned, given the size. |
I'm eager to get 5.7 out this weekend :) |
This PR covers a number of issues regarding concurrency and multi-threading. It is based on the recent BlockHound PR and offers an alternative to #3604. Although this is a lot, I thought it would be better to put it in the open for everyone to consider and discuss.
I am currently examining the further evolution of concurrent testing around existing, but less documented parameters like
concurrentSpecs
andconcurrentTests
. This PR intends to provide a sound basis for that.Details addressed in this PR:
Avoid the use of
runBlocking
inside coroutines. Schedule all coroutines below the test engine layer via regular dispatchers, bringing the test environment more in line with actual production environments.Remove direct use of Java's
Executors
, delegating to kotlinx.coroutines-provided dispatchers instead.Preserve coroutine context (which includes lots of Kotest configuration) in multithreaded test executions.
Remove hard-coded 1-day time limit for multithreaded test executions.
Change tests relying on single-threading from a coroutine model to a thread model, removing coroutine invocations (
delay
), replacing those with thread invocations (Thread.sleep
).Avoid leaking threads by closing dispatchers after use.
Keep the
assertionCounter
synchronized with thread-switching coroutines.Remove
fun <K, V> concurrentHashMap(): MutableMap<K, V>
, which was not thread-safe.Make
FixedThreadCoroutineDispatcherFactory.dispatcherAffinity
thread-safe.Revert a change in
ConcurrentTestSuiteScheduler.schedule
by commit c316bbd, which replacedlaunch
withasync
plusjoinAll
. This was functionally equivalent, butasync
was not needed, as there were no values returned, andjoinAll
was already performed by the enclosingcoroutineScope
.Fixes #3022