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 :(

draft for eventually

b007e0b

mirageoasis mentioned this pull request May 14, 2024

eventually without customization never runs test method #3988

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eventually fixed #4018

eventually fixed #4018

mirageoasis commented May 14, 2024 •

edited

mirageoasis commented May 14, 2024

OliverO2 May 14, 2024 •

edited

mirageoasis May 15, 2024

OliverO2 May 14, 2024

mirageoasis May 15, 2024

mirageoasis May 15, 2024

AlexCue987 May 17, 2024

mirageoasis May 17, 2024

OliverO2 May 17, 2024

mirageoasis May 18, 2024

OliverO2 May 18, 2024

AlexCue987 May 18, 2024

mirageoasis May 18, 2024

kshired left a comment

kshired May 19, 2024

sksamuel May 19, 2024

mirageoasis May 19, 2024

sksamuel May 19, 2024

mirageoasis commented May 19, 2024

OliverO2 May 20, 2024

mirageoasis May 20, 2024

mirageoasis May 20, 2024 •

edited

	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")

eventually fixed #4018

Are you sure you want to change the base?

eventually fixed #4018

Conversation

mirageoasis commented May 14, 2024 • edited

mirageoasis commented May 14, 2024

OliverO2 May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshired left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirageoasis commented May 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirageoasis May 20, 2024 • edited

Choose a reason for hiding this comment

mirageoasis commented May 14, 2024 •

edited

OliverO2 May 14, 2024 •

edited

mirageoasis May 20, 2024 •

edited