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 Clock.System() replaceable / mockable #33

Closed
LeoColman opened this issue Aug 14, 2020 · 7 comments
Closed

Make Clock.System() replaceable / mockable #33

LeoColman opened this issue Aug 14, 2020 · 7 comments

Comments

@LeoColman
Copy link

LeoColman commented Aug 14, 2020

Instead of injecting a Clock in every single function or class that uses the usual approach of LocalDateTime.now(), consider adding a way to replace the System clock instance if one wants to.

This would make classes that depend on the abstract context of now easily testable, and idiomatic, as we wouldn't need to inject it everywhere just for the sake of testing.

I'm happy to provide a pull request for this possibility

@LeoColman
Copy link
Author

Some similar behavior is present in some of Kotlin's libraries.

Fuel, for example enables changing the base instance

Kotest supports simulating the current time through Mockk as a way to make testing datetimes easier without having to inject a clock

@LeoColman
Copy link
Author

LeoColman commented Aug 14, 2020

Python also has a way to simulate date and time for testing without having to inject a dummy instance.

The same is possible with javascript and typescript, also discussed in this jestjs/jest#2234

@LeoColman
Copy link
Author

LeoColman commented Aug 14, 2020

@JakeWharton
Do you think this is a bad idea? If you do, could you tell me what points you see as flawed in this approach that I might have missed?

@JakeWharton
Copy link

I thought is was a bad idea for coroutines Dispatchers and for Rx Schedulers and for arch components executors (which I thankfully don't use) and for every other static setter I've ever used in the history of programming.

This is one of those decisions where people decry things like boilerplate and then spend the next five years complaining about test setup and non-determinism.

We have a mechanism to make this straightforward and deterministic and it's inversion of control. Just supply the clock like you supply everything else that's a dependency. Nothing about a clock is special here, so why treat it as such? I don't want to have to ban the static clock with its mutable backing implementation the same way I do with Dispatchers and Schedulers. Just pay the parameter cost, it's cheap!

@LeoColman
Copy link
Author

Why other programming languages and frameworks implemented date and time this way and people always ask for date mocking specifically?

@kluever
Copy link

kluever commented Aug 14, 2020

JodaTime has similar functionality (DateTimeUtils.setCurrentMillisFixed(long) and friends) and it's been a nightmare in large code systems. If some how one of your dependencies manages to use this in production, you're going to have a really bad time. We went as far as using Error Prone to prevent non-test code from using these APIs.

If you want to control the value of "now", then use dependency injection and inject a Clock into your system under test.

Static mutable state is almost always a bad idea.

@dkhalanskyjb
Copy link
Collaborator

Looks like the consensus is against the static setter idea, so closing the issue. The lesson from this discussion for us, the library writers, is that people do want to test their code using a mocked Clock.System. In practice, this means that we should strive to keep the surface area of the system-dependent subset of our API small, attempting to present as much functionality as is reasonable using pure calculations that never need to be mocked. Thank you all for the discussion!

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