Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make CircuitBreaker delay time mockable for testing #357

Open
dugsmith opened this issue Feb 20, 2023 · 6 comments
Open

Make CircuitBreaker delay time mockable for testing #357

dugsmith opened this issue Feb 20, 2023 · 6 comments

Comments

@dugsmith
Copy link

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
Copy link
Member

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
Copy link
Contributor

Tembrel commented Feb 20, 2023

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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:

  • You changed to use java.time.Instant.now() instead of System.nanoTime() so that I could do a static mock of java.time.Instant.
  • You made it possible to inject a java.time.Clock as a dependency, which I could then mock and provide to the class that implements my feature.

@stevenschlansker
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants