Schedule.recurs flaky test #29
Conversation
| "Schedule.recurs(n = 0)" { | ||
| forFew(2, Gen.intSmall()) { | ||
| val i = 0 | ||
| val res = Schedule.recurs<ForId, Int>(Id.monad(), i).runIdSchedule(0, i + 1) | ||
| res.isEmpty() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Shouldn't this be covered in line 185? Looks to me as if this does the same thing...
There was a problem hiding this comment.
Yes it does, the problem is that AFAIk this specific case with n=0 was failing for some reason.
There was a problem hiding this comment.
Yes, but you can see above I filtered out that case just to make it explicit in the test for n=0. The test was sometimes failing because of the generator making n=0.
What should we do here? Iirc with:
val res = Schedule.recurs<ForId, Int>(Id.monad(), i).runIdSchedule(0, i)
wasn't failing
There was a problem hiding this comment.
Schedule.recurs<ForId, Int>(Id.monad(), i).runIdSchedule(0, i + 1) is 100% deterministic (or at least it should be) so if the test fails with i == 0 something is definitly wrong! What error does it fail with i == 0?
There was a problem hiding this comment.
arrow.fx.ScheduleTest > Schedule.recurs(n = 0) FAILED
java.lang.AssertionError: Property failed for
86543)
at arrow.test.laws.LawKt.forFew(Law.kt:34)
at arrow.fx.ScheduleTest$6.invokeSuspend(ScheduleTest.kt:192)
at arrow.fx.ScheduleTest$6.invoke(ScheduleTest.kt)
There was a problem hiding this comment.
Basically runIdSchedule(0, max(2, i + 1))
There was a problem hiding this comment.
Yes this is actually a flaw in the test. Schedule.runIdSchedule(*, n) should always return n decisions. That is a design part of schedule: Unless it errors out in F (which it can't in the tests because F ~ Id) it will always return a decision. This means the test needs to be run n + 1 times for every n >= 0 and at least once for every other n. Also needs to add a check that checks if the last decision is a decision to stop
There was a problem hiding this comment.
so runIdSchedule(0, max(1, n + 1)) should suffice
There was a problem hiding this comment.
great! thanks for explaining it!
Let me quickly change it!!
There was a problem hiding this comment.
Now it fails after 1st attempt.. none of the options worked
| forAll(Gen.intSmall().filter { it < 1000 && it != 0 }) { i -> | ||
| val res = Schedule.recurs<ForId, Int>(Id.monad(), i).runIdSchedule(0, i + 1) | ||
| forAll(Gen.intSmall().filter { it < 1000}) { i -> | ||
| val res = Schedule.recurs<ForId, Int>(Id.monad(), i).runIdSchedule(0, max(1, i + 1)) |
There was a problem hiding this comment.
| val res = Schedule.recurs<ForId, Int>(Id.monad(), i).runIdSchedule(0, max(1, i + 1)) | |
| val res = Schedule.recurs<ForId, Int>(Id.monad(), i).runIdSchedule(0, max(2, i + 1)) |
There was a problem hiding this comment.
This is not the solution. The test assumption below is wrong. It needs to be changed to check if the last decision is false and all the prior ones are the true and have the correct result
There was a problem hiding this comment.
And it needs to do that in any case, even if n <= 0
@aballano happen to find this test failing sometimes see #2049
I found out the error case so I split the test into two just to discuss the implementation of it.