Add interface for Temporal class 🔧

[michael-rubel]

Hi @cappuc!

To be able to use dependency injection instead of facade and test classes in isolation we need an interface for the main Temporal class.

I haven't added types for those methods since all of them return concrete classes and that would have been a BC break (used generic object instead).

[cappuc]

Why is it a problem to set return types to the builder classes?
Do you need to override the builder classes?

[michael-rubel]

@cappuc in the future, you might return interfaces instead of concrete classes, like it was done in the original Temporal SDK (for example WorkflowRunInterface). This way you won't need to specify concretes in your implementation, so you can unit test it with your own fakes without pain. 🙂

[cappuc]

Changing the return type from object to an interface is a breaking change anyway, so I prefer to set the return type explicit.
Right now the only methods available returns the builder classes and I don't see the need to override them in the near feature.
So I think it's better to specify the return type because it improves DX (telling the correct return type to IDE) and if we need to change the return types, the required changed in the customers applications can be detected with a static analysis tool

[michael-rubel]

In this case, unit testing in isolation won't be possible, since concretes are returned, and you'll need to implement your fake using these concretes.

I think I can create an interface for each class and change these return types to make it unit-testable, but you'll need to ship it with a warning for others that the new release contains BC. Would you want to go this way?

[cappuc]

Can you post an example?

I can't understand why you need to mock the builder classes.
In order to unit test an Activity class, you can simply create an instance (without the ActivityBuilder) and test the method call.
To unit test Workflow class, you can build the workflow with the standard WorkflowBuilder, execute it through a temporal server, and check the result (if you need to mock activities result, you can with Temporal::fake() and Temporal::mockActivity()

[michael-rubel]

@cappuc what you're describing are feature tests since it executes directly on the infrastructure. I mean the tests that are testing my code in isolation (coverage, "class configuration" logic) and fully independent from the framework, database, or Temporal itself.

[michael-rubel]

For example:

---

[cappuc]

Why do you need the TemporalFake class for this tests?

I think you only need to mock the TemporalClientInterface in order to prevent running the workflow

[michael-rubel]

Yeah, but what if someone needs to swap the implementation of the stub builder? I'm used to mocking everything in unit tests. 😄

If you don't want to move in this direction, I don't insist though. Will abstract that on my own.
Only thought it might be helpful for someone who uses this package as well as I am.

[cappuc]

I need to think about it for a moment.
I don't want to add to much complexity with an interface for each class... maybe making the builder classes non-final can be sufficient?

[cappuc]

I've made some changes:

• Renamed the new Contract interface to Contracts\Temporal
• Added binding to the Temporal class, and used as Facade accessor
• Removed the final keyword from Builder classes so they can be extended.

Can it work for your use case?

[michael-rubel]

It's way better now, although not ideal for my case since all the builder fakes would need to be coupled to the concretes, but as you mentioned that would require a review of all used classes and return types in the package, so maybe in the future. :)

Thank you for the effort!