Added a class to abstract time to a centralised place to improve testing.

[mk-verma]

Pull Request type

• Bugfix
• Feature
• Refactoring (no functional changes, no api changes)
• Build related changes
• WHOSUSING.md
• Other (please describe): Improve testing.

NOTE: Please remember to run ./gradlew spotlessApply to fix any format violations.

Changes in this PR

We have a use case where we need to change time during execution of integration tests. This is an attempt to abstract out the time in conductor to a single place, so that during testing we can manipulate the time as per the need.

Describe the new behavior from this PR, and why it's needed
Issue #482
This PR adds a service to fetch current time in conductor via static methods. These methods can be overriden in tests to manipulate time.

Alternatives considered
I also thought of using Bean (via @bean config) to return clock instance or having TimeService as a Component but that would then require injecting the clock or TimeService into different classes which fetch current time. This breaks the backward compatibility and hence rejected those options.

[AWOrpington]

You're on the right track with removing System.currentTimeMillis, but it should be replaced directly with Java.time.Clock which then can be manipulated directly in tests.

[AWOrpington]

Generally classes shouldn't know about how they're used, especially with relation to testing.

[c4lm]

There are also places in Conductor which invoke Instant.now(), LocalDate.now() etc.
Could you please describe in more detail what kind of tests you are trying to run?

[mk-verma]

@c4lm

There are also places in Conductor which invoke Instant.now(), LocalDate.now() etc.

Yeah, I missed it actually 😓 . I will cover them as well

Could you please describe in more detail what kind of tests you are trying to run?

We have a use case in which we want to manipulate time during testing in one of our service which uses conductor-oss as self hosted orchestration engine. We want to use custom clock which is shared across all the direct dependencies in service, that is conductor, queue etc. This clock then be used to manipulate time. This is mainly be used in integration tests.

[mk-verma]

You're on the right track with removing System.currentTimeMillis, but it should be replaced directly with Java.time.Clock which then can be manipulated directly in tests.

Yes, but there are some problem to that.
The first approach that I thought was using @bean which returns the Clock. In production, this returns normal clock. In tests ,it can return mocked clock. But we need to be injected in constructor of each class where ever we need time. This poses backward compatibility issues as some of them (classes) are extendible by the services that use conductor.
For example WorkflowExecutorOps uses time and we need to modify the current constructor of add that clock bean. If we do so then the services which have their own WorkflowExecutorOps will break on version bump. If we create another constructor with clock, we need to somehow manage the Injection.
I am not sure about @Autowired on field/setter though. Let me know your thoughts.

[v1r3n]

I think overall this is a good idea. The approach I think that makes sense here is as the following:

1. Have a @bean that provides Clock with system default
2. Tests can override this clock provider to inject custom clock, update the ticks etc
3. Replace usage of system.currentTimeInMillis, Instant.now() etc with the clock.