Implement test timeouts #233
Conversation
| implicit def munitIoRuntime: IORuntime = IORuntime.global | ||
| @deprecated("Use munitIORuntime", "2.0.0") | ||
| def munitIoRuntime: IORuntime = IORuntime.global | ||
| implicit def munitIORuntime: IORuntime = munitIoRuntime: @nowarn |
There was a problem hiding this comment.
This wasn't necessary, but seemed better for consistency.
There was a problem hiding this comment.
Sorry for being grumpy, but I don't quite get these changes. Could you please elaborate on what we're about to achieve?
There was a problem hiding this comment.
No problem, this one is maybe controversial :) I was just feeling grumpy about munitIoRuntime instead of munitIORuntime, it was an eyesore for me.
Especially now that I add munitIOTimeout. Maybe it should be munitIoTimeout instead?
There was a problem hiding this comment.
Oh, Geez, I didn't recognize these IO -> Io changes. Shame on me 🙈. Although, I'm happy with the IO suffixes because we're referring to cats.effect.IO.
There was a problem hiding this comment.
Yes, right, I prefer IO too, so I changed from Io -> IO :)
| def munitIOTimeout: Duration = 30.seconds | ||
|
|
||
| // buys us a 1s window to cancel the IO, before munit cancels the Future | ||
| override def munitTimeout: Duration = munitIOTimeout + 1.second |
There was a problem hiding this comment.
Bit of trickiness here: an IO may still hang on cancellation, for various reasons. So we do still want the munit timeout in effect too, as a backup. 1 second seemed like a reasonable default to wait for cancellation before taking more drastic measures.
There was a problem hiding this comment.
It's a great note! I'd love to move it to scaladoc of munitTimeout. Also, it could be worth noting that IO can be uncancelable in its nature.
There was a problem hiding this comment.
Thanks! Good point, let me add some scaladocs.
| // TODO cleanup after CE 3.4.0 is released | ||
| val fd = Some(munitIOTimeout).collect { case fd: FiniteDuration => fd } | ||
| val timedIO = fd.fold(unnestedIO) { duration => | ||
| unnestedIO.timeoutTo( | ||
| duration, | ||
| IO.raiseError(new TimeoutException(s"test timed out after $duration")) | ||
| ) | ||
| } |
There was a problem hiding this comment.
In CE 3.4.0 we'll be able to directly pass a non-finite Duration to timeoutTo.
| implicit def munitIoRuntime: IORuntime = IORuntime.global | ||
| @deprecated("Use munitIORuntime", "2.0.0") | ||
| def munitIoRuntime: IORuntime = IORuntime.global | ||
| implicit def munitIORuntime: IORuntime = munitIoRuntime: @nowarn |
There was a problem hiding this comment.
Sorry for being grumpy, but I don't quite get these changes. Could you please elaborate on what we're about to achieve?
| override def munitIOTimeout = 100.millis | ||
| override def munitTimeout = Int.MaxValue.nanos // so only our timeout is in effect | ||
|
|
||
| test("times out".fail) { IO.sleep(1.second) } |
There was a problem hiding this comment.
Probably we could intercept here the timeout exception to make the test suite more precise. WDYT?
There was a problem hiding this comment.
Oh sorry, I missed this comment. Actually this doesn't work: the .intercept would apply only to the IO.sleep(1.second), which does not throw an exception. The timeout exception is being raised outside of the user-written test, so there is no way we can intercept it in a user-written test.
| def munitIOTimeout: Duration = 30.seconds | ||
|
|
||
| // buys us a 1s window to cancel the IO, before munit cancels the Future | ||
| override def munitTimeout: Duration = munitIOTimeout + 1.second |
There was a problem hiding this comment.
It's a great note! I'd love to move it to scaladoc of munitTimeout. Also, it could be worth noting that IO can be uncancelable in its nature.
| /** The timeout for [[cats.effect.IO IO]]-based tests. When it expires it will gracefully cancel | ||
| * the fiber running the test and invoke any finalizers. | ||
| * | ||
| * Note that the fiber may still hang while running finalizers or even be uncancelable. In this | ||
| * case the [[munitTimeout]] will take effect, with the caveat that the hanging fiber will be | ||
| * leaked. | ||
| */ | ||
| def munitIOTimeout: Duration = 30.seconds | ||
|
|
||
| /** The overall timeout applicable to all tests in the suite, including those written in terms of | ||
| * [[scala.concurrent.Future Future]] or synchronous code. This is implemented by the MUnit | ||
| * framework itself. | ||
| * | ||
| * When this timeout expires, the suite will proceed without waiting for cancelation of the test | ||
| * or even attempting to cancel it. For that reason it is recommended to set this to a greater | ||
| * value than [[munitIOTimeout]], which performs graceful cancelation of | ||
| * [[cats.effect.IO IO]]-based tests. The default grace period for cancelation is 1 second. | ||
| */ | ||
| override def munitTimeout: Duration = munitIOTimeout + 1.second |
CE 3.4 no longer requires to convert `munitIOTimeout` into a `FiniteDuration` See: typelevel#233
FTR, MUnit already supports timeouts, but it comes with caveats:
.canceltheIO, so finalizers are not run. Indeed, theIOdoesn't even stop running, the test suite just continues!By implementing first-class support for timeouts in munit-cats-effect we can solve both of these problems.