eventually fixed #4018

mirageoasis · 2024-05-14T01:38:56Z

will resolve #3988

this is draft for eventually

I made some senario for this new eventually

duration is set to positve -> works well
duration is set to minus-> error (this pr)
retries is set to minus -> error (this pr)
user do not set duration -> which make infinite loop (this pr)

and there are some other feature I introduced in this pr

if duration millisecond overflowed it is set to Duration.INFINITE and prints out waring

~~by the way can we change retries to Long? I think it doesn't matter to change Int to Long~~

sorry found out reason why retries cannot be Long in ExponentialIntervalFn.kt

mirageoasis · 2024-05-14T16:05:30Z

currently working on test code
Any suggestions are welcome!

OliverO2 · 2024-05-14T16:36:39Z

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

@@ -205,7 +211,7 @@ object NoopEventuallyListener : EventuallyListener {
 private class EventuallyControl(val config: EventuallyConfiguration) {

   val start = timeInMillis()
-   val end = start + config.duration.inWholeMilliseconds
+   val end = addSafely()


~~Have you tried to run this? The local start variable is not visible in addSafely(), right?~~
[Edit: It is visible. My bad. Just needed to look at the whole file instead of the GitHub diff.]

What about expressing more clearly what happens, so that readers don't have to look up the implementation of addSafely()? For example, mirroring the coerce... functions in the stdilb, we could use:

val end = (start + config.duration.inWholeMilliseconds).orInfinityIfNegative()

with:

fun Long.orInfinityIfNegative() = if (this < 0) Duration.INFINITE.inWholeMilliseconds else this

thanks to your advice I refactored code! thanks a lot!

OliverO2 · 2024-05-14T16:44:00Z

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

-   fun hasAttemptsRemaining() = timeInMillis() < end && iterations < config.retries
+   fun hasAttemptsRemaining() : Boolean {
+      // If the duration is infinite, it is considered as default value, which means it will never stop
+      val isInfinite = config.duration == INFINITE


Isn't this case already covered by isWithinDuration?

well

Isn't this case already covered by isWithinDuration?

isWithinDuration only checks time depletion
isInfinte checks if config.duration if is set to default which is INFINITE

oh maybe since Duration.INFINITE is almost same as INFINITE I think I can rollback this expression

…tsRemaining

AlexCue987 · 2024-05-17T15:16:20Z

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

@@ -121,7 +121,7 @@ data class EventuallyConfiguration(
 }

 object EventuallyConfigurationDefaults {
-   var duration: Duration = Duration.INFINITE
+   var duration: Duration = INFINITE


I am concerned over this approach in general. This thing is shared (because object) and mutable (because var). As such, there will be race conditions. We should refactor away the very need for shared mutable things. WDYT @OliverO2

since this is default setting I think it is ok to change var to val

@AlexCue987 I agree.

All properties should really be val (though I wonder how this would affect backward compatibility in this case). Global state is almost never a good idea and in particular with the option of running parallel tests and concurrent coroutines, any context-induced configuration should be coroutine-local. See #2791 for an example.

I also wonder why that object is part of the public API. It might better be private.

I think its appropriate to make another pr and deal with this issue
on this pr I will only focus on eventually's function
I will raise another pr for this issue!

Yes, that makes sense. To make this PR ready for review, we'd need some test code covering the proposed fixes. Would also be good to update the initial PR comment with references to issues resolved by this PR.

I think its appropriate to make another pr and deal with this issue on this pr I will only focus on eventually's function I will raise another pr for this issue!

sure - this is a preexisting condition, not something introduced in this PR

Yes, that makes sense. To make this PR ready for review, we'd need some test code covering the proposed fixes. Would also be good to update the initial PR comment with references to issues resolved by this PR.

done with test!

…ssage

kshired

@mirageoasis

I commented some suggestions.
Why not modify the comment of EventuallyConfigurationBuilder.duration that the constraint has been added?

kshired · 2024-05-19T02:44:34Z

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

+      return when {
+         this in 0..INFINITE.inWholeMilliseconds -> this
+         else -> INFINITE.inWholeMilliseconds.also {
+            println("[WARN] end value $this is out of the valid range (0 to ${INFINITE.inWholeMilliseconds}), value is fixed to Duration.INFINITE.inWholeMilliseconds")


Suggested change

println("[WARN] end value $this is out of the valid range (0 to ${INFINITE.inWholeMilliseconds}), value is fixed to Duration.INFINITE.inWholeMilliseconds")

println("[WARN] end value $this is out of the valid range (0 to $it), value is fixed to Duration.INFINITE.inWholeMilliseconds")

Should we not just error out and throw if the duration is invalid?

think throwing error is more clear then changing value since people will not set duration close to inifnite

And no one is going to spot the warn in the logs unless they look closely.
Failing the test makes more sense if its a bug, which having an incorrect duration is.

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

mirageoasis · 2024-05-19T16:48:41Z

Can someone help me with test case to trigger Duration overflow?

OliverO2 · 2024-05-20T10:40:13Z

...t-assertions-core/src/jvmTest/kotlin/io/kotest/assertions/nondeterministic/EventuallyTest.kt

+      }
+
+      test("when duration is set to default it cannot end test until iteration is done") {
+         val finalCount = 10000


This would also work with a count of 10, and faster, right?

mirageoasis · 2024-05-20T14:54:19Z

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

@@ -205,7 +211,7 @@ object NoopEventuallyListener : EventuallyListener {
 private class EventuallyControl(val config: EventuallyConfiguration) {

   val start = timeInMillis()
-   val end = start + config.duration.inWholeMilliseconds
+   val end = (start.milliseconds + config.duration).inWholeMilliseconds


here's how it works

Duration's single value cannot be Bigger than 2 ^ 63 - 1

if we add two Duration and if it's value's result is bigger than 2 ^ 63 -1 then we set Duration to Duration.INFINITE

so

this

val end = (start.milliseconds + config.duration).inWholeMilliseconds
is 100% safe!

I've come a long way round :(

mirageoasis · 2024-07-02T15:07:14Z

#4114
since this pr collides with this pr I will check what has changed!

mirageoasis · 2024-07-02T15:36:58Z

think conflicting pr now resolves overflow issue I think job of this pr is to limit duration and retries from being negative

mirageoasis · 2024-07-05T03:58:39Z

@aSemy
thank to your PR duration overflow issue has been solved
but I think retries and duration should not be negative since it can cause some error
can you review if my fixed pr does not collide with pr of your's?

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

aSemy · 2024-07-09T20:44:29Z

@aSemy thank to your PR duration overflow issue has been solved but I think retries and duration should not be negative since it can cause some error can you review if my fixed pr does not collide with pr of your's?

It looks like there are no conflicts 👍

However, there's lots of formatting changes that could cause conflicts in other PRs, and it makes the changes in the PR more noisy and harder to review. Would you mind reverting the formatting changes?

E.g. Revert

val result =
   testEventually(5.days) {

back to

val result = testEventually(5.days) {

AlexCue987 · 2024-07-09T21:48:23Z

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

@@ -133,7 +138,7 @@ object EventuallyConfigurationDefaults {
 class EventuallyConfigurationBuilder {

   /**
-    * The total time that the [eventually] function can take to complete successfully.
+    * The total time that the [eventually] function can take to complete successfully. Must be greater than or equal to 0.


maybe just "Must be non-negative"?

I will fix this on evening!

tbh I find "Must be greater than or equal to zero" easier to read, but "Must be non-negative" is fine too.

mirageoasis · 2024-07-10T11:49:10Z

think it is now okay to merge can someone approve my change?

aSemy

lgtm!

I have some minor nitpicks. Please double check them, and if they make sense to you, apply them before merging.

aSemy · 2024-07-12T23:04:26Z

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

@@ -202,7 +207,7 @@ class EventuallyConfigurationBuilder {
 typealias EventuallyListener = suspend (Int, Throwable) -> Unit

 object NoopEventuallyListener : EventuallyListener {
-   override suspend fun invoke(iteration: Int, error: Throwable) {}
+   override suspend fun invoke(iteration: Int, error: Throwable, ) {}


Suggested change

override suspend fun invoke(iteration: Int, error: Throwable, ) {}

override suspend fun invoke(iteration: Int, error: Throwable) {}

formatting nitpick

I personally like trailling comma but since it ruin's commit's visibility I will fix this!

I like trailing commas too, but only if the parameters are on separate lines :)

aSemy · 2024-07-12T23:05:19Z

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

@@ -156,7 +161,7 @@ class EventuallyConfigurationBuilder {
   var intervalFn: DurationFn? = EventuallyConfigurationDefaults.intervalFn

   /**
-    * The maximum number of invocations regardless of durations. By default, this is set to max retries.
+    * The maximum number of invocations regardless of durations. By default, this is set to max retries. And Must be greater than or equal to 0.


Suggested change

* The maximum number of invocations regardless of durations. By default, this is set to max retries. And Must be greater than or equal to 0.

* The maximum number of invocations regardless of durations. By default, this is set to max retries. Must be non-negative.

nitpick: match the KDoc of duration.

...t-assertions-core/src/jvmTest/kotlin/io/kotest/assertions/nondeterministic/EventuallyTest.kt

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt

mirageoasis · 2024-07-13T04:29:12Z

should I fix docs or leave docs cause my change and limitation is too obvious?

mirageoasis · 2024-07-14T03:16:14Z

@Kantis Can you tell me why my test fails? is it my fault? or problem with configuration?

when I see logs it says some kind of kotlin dependency

OliverO2

Looks good, ready to merge. Thank you for this contribution!

draft for eventually

b007e0b

mirageoasis mentioned this pull request May 14, 2024

eventually without customization never runs test method #3988

Closed

mirageoasis marked this pull request as ready for review May 14, 2024 16:04

OliverO2 reviewed May 14, 2024

View reviewed changes

mirageoasis added 2 commits May 16, 2024 00:44

refactor: refactored range check function and rolled backed hasAttemp…

a41907c

…tsRemaining

Merge branch 'master' into new_eventually

fcfe52b

AlexCue987 reviewed May 17, 2024

View reviewed changes

mirageoasis added 4 commits May 19, 2024 03:05

refactor: when duration is set to default it does not print waring me…

00d0ae9

…ssage

test: added test to cover required senario

803dc50

refactor: fixed error message

a0d0937

test: added test for error message

df7699c

mirageoasis changed the title ~~draft for eventually~~ eventually refactored May 19, 2024

Merge branch 'master' into new_eventually

fd57a11

kshired reviewed May 19, 2024

View reviewed changes

mirageoasis added 2 commits May 19, 2024 23:33

refactor: fixed comment following changes in constraint

f1d34d0

refactor: throw when end is not between range

a1038b8

OliverO2 reviewed May 20, 2024

View reviewed changes

mirageoasis added 2 commits May 20, 2024 23:37

feat: used duration plus to prevent overflows

45a7407

feat: wrote test for issue#3988

ca73518

mirageoasis commented May 20, 2024

View reviewed changes

Merge branch 'master' into new_eventually

946651d

mirageoasis force-pushed the new_eventually branch from b94febd to 946651d Compare May 21, 2024 11:31

mirageoasis changed the title ~~eventually refactored~~ eventually fixed May 23, 2024

Merge branch 'kotest:master' into new_eventually

9e94ea6

Kantis added the assertions 🔍 Related to the assertion mechanisms within the testing framework. label Jun 5, 2024

mirageoasis mentioned this pull request Jun 22, 2024

mutable Issue for EventuallyConfigurationDefaults #4147

Closed

conflict fixed

f3763df

fixed conflict and test accordingly

c11f6e8

aSemy reviewed Jul 9, 2024

View reviewed changes

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt Outdated Show resolved Hide resolved

aSemy reviewed Jul 9, 2024

View reviewed changes

...st-assertions-core/src/commonMain/kotlin/io/kotest/assertions/nondeterministic/eventually.kt Outdated Show resolved Hide resolved

AlexCue987 reviewed Jul 9, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

mirageoasis added 2 commits July 10, 2024 20:44

refactor: reduced conflict formatting

ce60121

refactor: fixed error message

bedbe93

refactor: fixed comment

effef0b

mirageoasis requested review from AlexCue987 and aSemy July 10, 2024 11:50

aSemy approved these changes Jul 12, 2024

View reviewed changes

mirageoasis added 3 commits July 13, 2024 10:55

refactor: minor nitpicks

28af6e7

Merge branch 'master' into new_eventually

a454f4a

refactor: minor nitpicks

f5ced58

mirageoasis added 2 commits July 20, 2024 23:30

chore: fixed conflict

77c35fe

fixed: rolled back changes

4e8321c

OliverO2 approved these changes Jul 20, 2024

View reviewed changes

OliverO2 added this pull request to the merge queue Jul 20, 2024

Merged via the queue into kotest:master with commit 0988a45 Jul 20, 2024
7 checks passed

mirageoasis deleted the new_eventually branch July 21, 2024 04:13

mirageoasis mentioned this pull request Jul 23, 2024

refactor: changed mutablity and visibility of EventaullyConfiguration #4211

Closed

	println("[WARN] end value $this is out of the valid range (0 to ${INFINITE.inWholeMilliseconds}), value is fixed to Duration.INFINITE.inWholeMilliseconds")
	println("[WARN] end value $this is out of the valid range (0 to $it), value is fixed to Duration.INFINITE.inWholeMilliseconds")

	override suspend fun invoke(iteration: Int, error: Throwable, ) {}
	override suspend fun invoke(iteration: Int, error: Throwable) {}

	* The maximum number of invocations regardless of durations. By default, this is set to max retries. And Must be greater than or equal to 0.
	* The maximum number of invocations regardless of durations. By default, this is set to max retries. Must be non-negative.

eventually fixed #4018

eventually fixed #4018

Uh oh!

Conversation

mirageoasis commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mirageoasis commented May 14, 2024

Uh oh!

OliverO2 May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kshired left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mirageoasis commented May 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirageoasis May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mirageoasis commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mirageoasis commented Jul 2, 2024

Uh oh!

mirageoasis commented Jul 5, 2024

Uh oh!

Uh oh!

Uh oh!

aSemy commented Jul 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

mirageoasis commented Jul 10, 2024

Uh oh!

aSemy left a comment

Choose a reason for hiding this comment

Uh oh!

mirageoasis commented May 14, 2024 •

edited

Loading

OliverO2 May 14, 2024 •

edited

Loading

mirageoasis May 20, 2024 •

edited

Loading

mirageoasis commented Jul 2, 2024 •

edited

Loading