Conversation
|
If you mind the updates, I can revert them all and hope for best |
TimWSpence
left a comment
TimWSpence
left a comment
There was a problem hiding this comment.
Hey @mr-git thanks so much for taking this on and for fixing the dependency updates, etc at the same time! I'm new to the project but have done my best to review it.
I hope it doesn't look like too much - most of the comments are just aimed at making the tests easier to read. Please let me know if any of them don't make sense!
| scalaJSLinkerConfig ~= { _.withModuleKind(ModuleKind.CommonJSModule)}, | ||
| ).nativeSettings( | ||
| tlVersionIntroduced := List("2.12", "2.13", "3").map(_ -> "0.5.1").toMap | ||
| tlVersionIntroduced := List("2.12", "2.13", "3").map(_ -> "1.15.0").toMap |
There was a problem hiding this comment.
Is this supposed to be changed? I thought this was set to whatever version at which we first introduced scala native support
| closed <- Ref[IO].of(false) | ||
| cb = cb1.doOnOpen(opened.set(true)).doOnClosed(closed.set(true)).doOnHalfOpen(halfOpened.set(true)) | ||
| dummy = new RuntimeException("dummy") | ||
| taskInError = cb.protect(IO[Int](throw dummy)) |
There was a problem hiding this comment.
More idiomatically this would be IO.raiseError(dummy)
| taskSlowSucceeds = cb.protect(IO.sleep(slowDuration)) | ||
| _ <- taskSlowSucceeds.start | ||
| _ <- cb.state.map { | ||
| case _: CircuitBreaker.Closed => assert(true) |
There was a problem hiding this comment.
I think we'll get better error messages if this is cb.state.map(st => assertEquals(st, CircuitBreaker.Closed))
There was a problem hiding this comment.
In fact, seeing the other assertions that you have, you could extract a
def expectState(expected: CircuitBreaker.State) = cb.state.assertEquals(expected)
That might make these tests a bit easier to read
| backoff = Backoff.constant(resetTimeout), | ||
| maxResetTimeout = resetTimeout | ||
| ) | ||
| opened <- Ref[IO].of(false) |
| _ <- taskSlowSucceeds.attempt | ||
| _ <- cb.state.map { | ||
| case _: CircuitBreaker.Closed => assert(true) | ||
| case x => println(x); assert(false) |
| case Left(_: CircuitBreaker.RejectedExecution) => assert(true) | ||
| case _ => assert(false) | ||
| } | ||
| _ <- IO.sleep(slowDuration + 10.milliseconds) // `taskSlowSucceeds` finishes after expiration and closes `cb` |
There was a problem hiding this comment.
Instead of sleeping here, we could actually save a handle to the fiber of the initial taskSlowSucceeds and then we could call fiber.join to wait the minimum amount of time here
| test | ||
| } | ||
|
|
||
| test("Validate behaviour for one slow call followed by fast calls"){ |
There was a problem hiding this comment.
Same comments as above. Maybe if we renamed taskSlowSucceeds to something like taskFasterThanResetTimeout (still a bit wordy - maybe we can do better) then the intention would be clearer? I initially assumed that taskSlowSucceeds would take longer than the reset timeout and got confused what this was testing
There was a problem hiding this comment.
Maybe this should be named something like Should not close circuit again before reset timeout has elapsed?
2 participants
My humble attempt to fix premature closing, though I am challenged by Scala.js
For local testing, I had to change Scala versions to latests (I am using only JDK 21), but I can drop those changes, if this will compile.
Not sure, but IMHO it would be nice to update to latest Cats Effect 3.5.x, thus I updated version from
0.5to0.6.I can drop all of those updates!