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

Replace until with eventually and make eventually more configurable #2022

Merged
merged 8 commits into from Jan 30, 2021
Merged

Replace until with eventually and make eventually more configurable #2022

merged 8 commits into from Jan 30, 2021

Conversation

jschneidereit
Copy link
Member

Fixes #2019

@jschneidereit jschneidereit self-assigned this Jan 27, 2021
build.gradle.kts Outdated
kotlinOptions.jvmTarget = "1.8"
kotlinOptions.apiVersion = "1.4"
kotlinOptions {
freeCompilerArgs = freeCompilerArgs + listOf("-Xopt-in=kotlin.RequiresOptIn", "-Xopt-in=kotlin.time.ExperimentalTime")
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if this doesn't work because of something related to buildSrc 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Does it not compile or ... ?

Copy link
Member

Choose a reason for hiding this comment

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

You can just list on multiple lines too

freeCompilerArgs = freeCompilerArgs + "-Xopt-in=kotlin.RequiresOptIn"
freeCompilerArgs = freeCompilerArgs + "-Xopt-in=kotlin.time.ExperimentalTime"

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't get rid of the warning about RequiresOptin and doesn't auto optin to ExperimentalTime unfortunately

Copy link
Member

Choose a reason for hiding this comment

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

I can't get it to work on MPP either

eventually(duration, interval, exceptionClass = exceptionClass, f = f)

@OptIn(ExperimentalTime::class)
@Deprecated("Use eventually with an interval, using Duration based poll is deprecated",
Copy link
Member

Choose a reason for hiding this comment

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

Can we put an example in the deprecatioin message, and add an extension method to turn a duration into an interval if we don't have one already

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, the first argument to ReplaceWith is an expression that describes how to replace the call site of the deprecated functions. But I also updated the message to include the fixed, fib, and exp calls to convert a Duration to an Interval 👍

@jschneidereit jschneidereit marked this pull request as ready for review January 30, 2021 04:26
@sksamuel
Copy link
Member

If it's ready to merge I will @jschneidereit

@jschneidereit jschneidereit changed the title initial work to replace until with eventually and make eventually more configurable Replace until with eventually and make eventually more configurable Jan 30, 2021
@jschneidereit jschneidereit merged commit a1c230f into kotest:master Jan 30, 2021
@jschneidereit jschneidereit deleted the jschneidereit/feature/2019-eventually branch January 30, 2021 19:51
@xeruf
Copy link
Contributor

xeruf commented Feb 4, 2021

Is there any documentation on how to replace existing until statements so they are equivalent (don't swallow exceptions and wait for predicate)?

@xeruf
Copy link
Contributor

xeruf commented Feb 4, 2021

Why is EventuallyPredicate a separate interface instead of a simple (T) -> Boolean lambda (or an equivalent typealias)?

@xeruf
Copy link
Contributor

xeruf commented Feb 4, 2021

Looks like the equivalent to

        until(duration, 40.milliseconds.fibonacci()) { messages.isNotEmpty() }

is

        eventually(duration, 40.milliseconds.fibonacci(), predicate = { messages.isNotEmpty() }) {}

quite a bit more verbose :/ especially dislike that empty lambda at the end

@jschneidereit
Copy link
Member Author

Ah, I suppose I forgot to make f have a default argument there

@xeruf
Copy link
Contributor

xeruf commented Feb 4, 2021

and the error is very unhelpful:

Eventually block failed after 4.00s; attempted 10 time(s); FibonacciInterval(base=40.0ms, offset=0) delay between attempts

java.lang.AssertionError: Eventually block failed after 4.00s; attempted 10 time(s); FibonacciInterval(base=40.0ms, offset=0) delay between attempts

with the above code - doesn't mention the predicate at all

@sksamuel
Copy link
Member

sksamuel commented Feb 4, 2021 via email

@xeruf
Copy link
Contributor

xeruf commented Feb 4, 2021

        eventually(duration, 40.milliseconds.fibonacci()) {
            messages.shouldNotBeEmpty()
        }

produces

java.lang.AssertionError: Eventually block failed after 2.00s; attempted 8 time(s); FibonacciInterval(base=40.0ms, offset=0) delay between attempts
The first error was caused by: Collection should not be empty
java.lang.AssertionError: Collection should not be empty
	at sc.server.client.PlayerListener$waitForMessage$1$1.invokeSuspend(PlayerListener.kt:36)
	at sc.server.client.PlayerListener$waitForMessage$1$1.invoke(PlayerListener.kt)
	at io.kotest.assertions.timing.EventuallyKt.eventually(eventually.kt:98)
	at io.kotest.assertions.timing.EventuallyKt.eventually$default(eventually.kt:87)
	at io.kotest.assertions.timing.EventuallyKt.eventually-CiwmDLY(eventually.kt:24)
	at sc.server.client.PlayerListener$waitForMessage$1.invokeSuspend(PlayerListener.kt:35)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
	at kotlinx.coroutines.EventLoopImplBase.processNextEvent(EventLoop.common.kt:274)
	at kotlinx.coroutines.BlockingCoroutine.joinBlocking(Builders.kt:84)
	at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking(Builders.kt:59)
	at kotlinx.coroutines.BuildersKt.runBlocking(Unknown Source)
	at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking$default(Builders.kt:38)
	at kotlinx.coroutines.BuildersKt.runBlocking$default(Unknown Source)
	at sc.server.client.PlayerListener.waitForMessage-QrpHQqc(PlayerListener.kt:34)
	at sc.server.client.PlayerListener.waitForMessage-QrpHQqc$default(PlayerListener.kt:34)
	at sc.server.network.RequestTest.stepRequest(RequestTest.kt:253)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:686)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
	at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:149)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:140)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:84)
	at org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.lambda$invoke$0(ExecutableInvoker.java:105)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:104)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:98)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$6(TestMethodTestDescriptor.java:212)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:208)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:137)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:71)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:135)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at java.util.ArrayList.forEach(ArrayList.java:1259)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at java.util.ArrayList.forEach(ArrayList.java:1259)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:32)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:51)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:248)
	at org.junit.platform.launcher.core.DefaultLauncher.lambda$execute$5(DefaultLauncher.java:211)
	at org.junit.platform.launcher.core.DefaultLauncher.withInterceptedStreams(DefaultLauncher.java:226)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:199)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:132)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:99)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:79)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:75)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:61)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at com.sun.proxy.$Proxy2.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.stop(TestWorker.java:133)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:182)
	at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:164)
	at org.gradle.internal.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:414)
	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
	at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:48)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:56)
	at java.lang.Thread.run(Thread.java:748)

The last error was caused by: Collection should not be empty
java.lang.AssertionError: Collection should not be empty
	at sc.server.client.PlayerListener$waitForMessage$1$1.invokeSuspend(PlayerListener.kt:36)
	at sc.server.client.PlayerListener$waitForMessage$1$1.invoke(PlayerListener.kt)
	at io.kotest.assertions.timing.EventuallyKt.eventually(eventually.kt:98)
	at io.kotest.assertions.timing.EventuallyKt$eventually$10.invokeSuspend(eventually.kt)

bit better, but still meh

@xeruf
Copy link
Contributor

xeruf commented Feb 4, 2021

suspend fun until(duration: Duration, interval: Interval = 1.seconds.fixed(), f: suspend () -> Boolean) =
   eventually(duration, interval, f = f)

The heck? These are not even close to equivalent!!

@xeruf
Copy link
Contributor

xeruf commented Feb 4, 2021

Just wanna say that I do appreciate your efforts at creating a more uniform interface here, but it seems this was a bit rushed ;)

@xeruf
Copy link
Contributor

xeruf commented Feb 4, 2021

To be fair, the error message for (in 4.3.2)

        until(duration, 40.milliseconds.fibonacci()) { messages.isNotEmpty() }

wasn't much more helpful:

Test failed after 4000ms; attempted 10 times
java.lang.AssertionError: Test failed after 4000ms; attempted 10 times
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTaskKt.resume(DispatchedTask.kt:176)
	at kotlinx.coroutines.DispatchedTaskKt.dispatch(DispatchedTask.kt:111)
	at kotlinx.coroutines.CancellableContinuationImpl.dispatchResume(CancellableContinuationImpl.kt:308)
	at kotlinx.coroutines.CancellableContinuationImpl.resumeImpl(CancellableContinuationImpl.kt:318)
	at kotlinx.coroutines.CancellableContinuationImpl.resumeUndispatched(CancellableContinuationImpl.kt:400)
	at kotlinx.coroutines.EventLoopImplBase$DelayedResumeTask.run(EventLoop.common.kt:489)
	at kotlinx.coroutines.EventLoopImplBase.processNextEvent(EventLoop.common.kt:274)
	at kotlinx.coroutines.BlockingCoroutine.joinBlocking(Builders.kt:84)
	at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking(Builders.kt:59)
	at kotlinx.coroutines.BuildersKt.runBlocking(Unknown Source)
	at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking$default(Builders.kt:38)
	at kotlinx.coroutines.BuildersKt.runBlocking$default(Unknown Source)

@jschneidereit
Copy link
Member Author

What would be helpful for these error messages, the eventually error message always produced a stack trace and I saw until not producing one as a gap.

I'm gonna try to convert this discussion into a new Issue with a list of action items in a minute here.

@sksamuel
Copy link
Member

sksamuel commented Feb 4, 2021 via email

@xeruf
Copy link
Contributor

xeruf commented Feb 4, 2021

What would be helpful for these error messages, the eventually error message always produced a stack trace and I saw until not producing one as a gap.

Somehow matchers need to properly integrated I guess

@jschneidereit
Copy link
Member Author

The matcher message is printed though?:

java.lang.AssertionError: Eventually block failed after 2.00s; attempted 8 time(s); FibonacciInterval(base=40.0ms, offset=0) delay between attempts
The first error was caused by: Collection should not be empty

@jschneidereit
Copy link
Member Author

@xerus2000 see #2046 - I added in what you were describing but I could not find before #2022 went in - a version of eventually where there is just a predicate and no producer. I added it as eventuallyPredicate but can be convinced to rename it to until 😂 as long as it stays in eventually.kt.

@xeruf
Copy link
Contributor

xeruf commented Feb 5, 2021

The matcher message is printed though?:

Yes, but it prints it in a quite convoluted way, and it currently produces an exception on each poll of eventually, not very efficient.

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.

Deprecate until and move its features to eventually
3 participants