failsafe-lib / failsafe

Fault tolerance and resilience patterns for the JVM
https://failsafe.dev
Apache License 2.0
4.16k stars 295 forks source link

Make CircuitBreaker delay time mockable for testing #357

Open dugsmith opened 1 year ago

dugsmith commented 1 year ago

I'm using CircuitBreaker in its Standalone Usage. My CircuitBreaker is configured withFailureRateThreshold(50, 2, Duration.ofSeconds(30)) to open when 50% of requests over a 30 second period fail.

I'd like to be able to unit test around the CircuitBreaker behavior so that I know that the closed / open / half-open behaviors are correct.

The challenge appears to be that OpenState uses System.nanoTime() to calculate the delay. This is very difficult to mock. I started going down the road of using AspectJ, but this is a mixed Kotlin/Java Android project, and the best library I found to implement AspectJ is no longer maintained because Android Gradle Plugin 8.0 will no longer support bytecode transformation.

If you switched to use something like java.time.Instant.now(), that would make test mocking much easier.

I see that in your own tests you just configure very short delay times and use Thread.sleep(). That's okay, but it's not nearly as flexible as being able to mock time itself.

jhalterman commented 1 year ago

Yes, I can imagine mocking System.nanoTime would be difficult, though I would think the same is true withInstant.now since they're both static - or am I missing something? From what I understand, the problem you're looking to avoid, using System time for unit tests, can only be avoided if we have some overridable mechanism for supplying the current time. Maybe you can give me a better idea of what you were thinking.

Tembrel commented 1 year ago

Something like the Clock abstraction might be useful, but it would mean some refactoring, because I don't think that it works at nanosecond granularity. (Mind you, I don't think one needs or wants true nanosecond granularity in cases where Failsafe is used, but the code currently is written that way.)

dugsmith commented 1 year ago

Thank you for the quick response, @jhalterman. System.nanoTime() is a native method, and MockK doesn't mock native methods. Instant.now() is static but not native, so it is easily mockable by libraries like MockK.

If you did want to provide a Clock abstraction for unit testing, that would be even better. But a quicker fix might be to just use java.time.Instant and document how to mock it.

jhalterman commented 1 year ago

I'm not sure I'd document mocking OpenState since it's an internal, package private class that isn't really meant to be used directly. Can you give me a better idea of how you're using it and testing against it?

dugsmith commented 1 year ago

I wasn't thinking about mocking OpenState. I was thinking of mocking java.time.Instant, unless you do provide a way to inject a Clock.

Here's an example of what I'd like to do in a test:

  1. Configure a CircuitBreaker to Open if 50% of requests fail over a 30 second period
  2. Setup test conditions for a certain time
  3. Invoke example requests that exercise the CircuitBreaker in a way that it should fail
  4. Set the time to 30 seconds in the future
  5. Assert that the CircuitBreaker has indeed entered the Open state

Ideally, I'd like the CircuitBreaker itself to not be exposed to the test, because it is an implementation detail of the overall feature I'm trying to implement. I'd just want the feature itself to behave like this because it is using the CircuitBreaker internally. I could accomplish that if either:

stevenschlansker commented 1 year ago

We would love to be able to test these sorts of behavior as well. We don't want to have to mock static classes - this gets ugly fast. It'd be great to plug-in a user supplied Clock (or InstantSource, but that's 17+). We already replace the Clock with many other pieces of code for testing purposes.