frankdejonge / testing-without-mocking-frameworks

29 stars 0 forks source link

Something that popped out into my mind: shouldn't "wasInvoiceSubmit" be on a different interface? #1

Open matheusgontijo opened 1 year ago

matheusgontijo commented 1 year ago

@frankdejonge

Thank you so much πŸ™πŸ™πŸ™ for writing up the Testing without mocking frameworks blogpost! Omgoodness, that cleared up a looooot of stuff for me in regards to testing without mocking! Can't thank you enough for such brilliant content. πŸ™ŒπŸ™ŒπŸ™ŒπŸ™Œ

I read and read the content... several times... and one specific thing really drew my attention. I reflected for a while about the wasInvoiceSubmit method on the InvoicePortal interface. Link here.

Let's say that I agree with everything you said, except for one thing that popped out into my mind. It's related to the design of interfaces in your last example. It could be something that I quite didn't understand it yet, but you might have a pretty good explanation for that πŸ™‚

Imagine the following scenario: we are NOT writing any tests. Yes, that's pretty bad thing to do, but just for the sake of the example, let's put tests aside for a second. Now, let's say that we are writing the InvoicePortal interface. It would only contain the submitInvoice method. That's all it needs... the submitInvoice method and no extra methods whatsoever. It should look like this:

interface InvoicePortal
{
    /**
     * @throws UnableToInvoiceClient
     */
    public function submitInvoice(Invoice $invoice): void;
}

So far, so good. Anybody that will use the InvoicePortal interface will then have a single method to call, correct? Example:

$invoicePortal->submitInvoice($invoice);

Here comes the problem: if we compare the above interface against the interface on the final example here, the wasInvoiceSubmit method was added to InvoicePortal.

Why? Just for testing purposes. Theorically wasInvoiceSubmit should not be there... as it is not part of the business per say. Business only care about submitInvoice. The wasInvoiceSubmit method is actually exclusively used for testing purposes, and nothing else.

Wouldn't it be better in terms of design if we create another interface (let's call it InvoicePortalTestContract - just a sample name, it could be better named, of course) that will extend the InvoicePortal? InvoicePortalTestContract will inherit all InvoicePortal methods and then on that interface we add all necessary methods (like wasInvoiceSubmit) in order to verify on our tests? Example:

interface InvoicePortalTestContract extends InvoicePortal
{
    public function wasInvoiceSubmit(Invoice $invoice): bool;
}

The greatest advantage of following this approach is that as the we add more methods down the road, we have separation of concerns like:

Only behaviours that business care:

interface InvoicePortal
{
    public function submitInvoice(Invoice $invoice): void;
    public function updateInvoice(Invoice $invoice): void;
    public function deleteInvoice(Invoice $invoice): void;
    public function markFlagInvoice(Invoice $invoice): void;
    public function linkInvoiceToOrder(Invoice $invoice): void;
}

Only test behaviours to be verified on tests:

interface InvoicePortalTestContract extends InvoicePortal
{
    public function wasInvoiceSubmit(Invoice $invoice): bool;
    public function wasInvoiceUpdated(Invoice $invoice): bool;
    public function wasInvoiceDeleted(Invoice $invoice): bool;
    public function wasMarkedFlagOnInvoice(Invoice $invoice): bool;
    public function wasInvoiceLinkedToOrder(Invoice $invoice): bool;
}

Then everything we need to make sure tests use the InvoicePortalTestContract instead of InvoicePortal, so tests can use all methods (business behaviours and tests behaviours). You can see a simple diff file here to what needs to be updated.

Does that make any sense? What would be your thoughts on that approach?

P.S.: Once again, thank you so much for your great content you put out! πŸ™πŸ™πŸ™

frankdejonge commented 1 year ago

Hey! First of all, thank you for the kind words. It's always great to hear when people enjoy my content.

As for the wasInvoiceSubmitted method, those kinds of methods serve more purposes than just testing. I'll explain why they could still be relevant in a production setting. But first, I'd like to state that the solution you provide can work just the same. I tend to put givenScenarioDescription (like givenSubmittingAnInvoiceTimesOut) methods on the test contract that the tests need to implement, but the end result is about the same. I would most likely not use all the wasX methods, but instead get a method to get the invoice properties and check for the expected state on the service's end.

Now, for the rationale: In this example the invoice portal is most likely a service external to the one we're developing. That means these services are connected over the network. With network connected services, and the request times out, what is the state of the external service? Has the invoice been submit or not? The answer is that we do not know. If/when these operations are not idempotent, we need to make them so on the integrating end. This means we'd account for the case when submitting the invoice results in a time-out, we can't safely retry, so instead we check for an existing invoice before retrying.

Hope this makes sense.

matheusgontijo commented 1 year ago

Awesome, @frankdejonge πŸ‘πŸ‘πŸ‘ Thank you so much for the explanation. That was clarifying πŸ’‘πŸ’‘πŸ’‘

Do you have any good open-source projects that follow your testing approach (or at least something similar) to send to me by any chance? I would love to dive deeply into the code and understand how they work in a large implementation.

Thank you so much for your time.