[WIP] Feature | V3 - Request validator

[bobmulder]

This PR introduces the RequestValidator as discussed in #275.

At the moment of writing, it's a first draft to align our ideas. After that, the missing checks and tests will be added.

I decided to create dedicated *Check classes. This way, they have their own responsibility, can be tested separately and can have more context (like message())

Questions to @Sammyjo20:

• Is there a strict folder structure? How do you feel about the current folder structure?
• Can you check how PendingRequests are supported?

The following checks are included:

• BodyEqualsCheck
• EndpointContainsCheck
• EndpointEndsWithCheck
• EndpointStartsWithCheck
• InstanceOfCheck
• QueryEqualsCheck

[Sammyjo20]

Hey @bobmulder thanks so much for working on this! 🙌 I'm a little busy tonight but I'll be taking a look over the weekend :)

[bobmulder]

Thats fine Sam. I just want to check with you if the general setup is right. I'll try to do the pending work next week.

[Sammyjo20]

Hey @bobmulder I've had a little look over and it looks good so far. Happy with the folder structure and I like your "check" contract - that looks good.

With regards to the PendingRequest - my worry is that if someone has used middleware, or a plugin to add a header, body or config then the original Request class won't reflect those changes. From v2, before a request is sent, a PendingRequest is instantiated and everything gets merged into there. It has a similar API to connectors and requests (e.g $pendingRequest->headers()). I would say it's probably more accurate to use this as the object that you're testing against rather than the Request class itself.

What are your thoughts?

I realise that this is something I think I have forgotten to update - the assertSent should also pass in the PendingRequest or maybe replace Request entirely in v3. I am happy to make this change, just let me know.

[bobmulder]

Yeeha @Sammyjo20!

I think you're completely right about the PendingRequest thing (you're the creator after all ;P), but I just don't see it. When I test a feature of my application, I just mock every request that's called, and do some assertions on every request individually using assertSent.

Can you tell me what should be changed? Should RequestValidator accept a PendingRequest only, and do all checks on that object instead of its underlaying Request object?

Without any knowledge about PendingRequest, I'd say that PendingRequest and Request should share the same (abstract) base class or an interface, which would make it less important what kind of request you're testing.

I'll get to know some more about pending requests today first.

[Sammyjo20]

Hey @bobmulder I've had a re-think about the structure of some of the testing helpers. Would you mind moving the testing validators into a "Testing" directory inside of the "Helpers" directory?

So we'll be able to use src/Helpers/Testing/RequestValidator.php?

With the PendingRequest stuff - it's just a more accurate class because if you want to assert that a header that existed on the connector was applied and merged into the sent request, the Request object wouldn't contain that header, so the PendingRequest is the single source of truth after a request has been sent. I think I might need to change the way the MockClient works so instead of a Request class it could return a PendingRequest in the closure?

Update: Just to clarify - here's what I'm thinking we could change it to.

Currently the assertSent works like this. You can either pass a request class or a closure. We'll keep the request class passing in - that's not a problem, but we'll maybe change the closure to have a PendingRequest returned instead?

$mockClient->assertSent(GetForgeServersRequest::class); // Stays the same

$mockClient->assertSent(function (PendingRequest $pendingRequest) {
     RequestValidator::make($pendingRequest)
            ->hasHeader()
            ->instanceOf()
            ->etc()
});

[Sammyjo20]

Hey @bobmulder - I hope you are well! I just had an idea. What if, instead of creating a RequestValidator from Request or PendingRequest, we could either use the same method or make a new method that passes in an already seeded RequestValidator that has already been given a PendingRequest? Then you could go straight in and do some validation?

$mockClient->assertSentWithValidate(function (RequestValidator $validator) {
     $validator
            ->hasHeader()
            ->instanceOf()
            ->etc()
});

[bobmulder]

Sorry for the delay, @Sammyjo20.

I think your idea is pretty nice, I love it. It's close to one of the suggestions I posted in the discussion. However, no matter how we would integrate the validator, we would need a validator class. Also, for testing purposes and to apply SRP I think it's a good idea to create a validator class which will do the heavy lifting, and create some 'glue' afterwards to integrate it nicely for testing (like your assertSentWithValidate idea).

The only thing I'm struggling right now is: should the validator class support Request classes or PendingRequest classes? Or should those classes be more bundled using an interface?

[Sammyjo20]

Hey @bobmulder apologies for the delay - with v3 launch and the book launch I decided to take a step away for a few weeks.

Would love to keep working on this with you - I'm glad that you like my suggestion :) I think we should use the pending request to be honest - that has the single source of truth and contains things merged from the connector/request and would be the closest to the actual request sent.

Or - what if you used the PSR-7 request? You can get it with $response->getPsrRequest() and there's nothing lower level than that so might be good to use.

[bobmulder]

Hi Sam, I don't blame you because I haven't done much on the PR as well ;).

I understand the PendingRequest is better to use instead of normal Request classes. But as a noob I'd say; why don't Request and PendingRequest share an interface?

Next week, I'll try to change my idea to supporting PendingRequest classes and I'll get back to you for some feedback and thoughts.

Wish you all the best!

[Sammyjo20]

No worries at all 🤠

It's a very fair point about the interface - I would be very happy to add an interface as long as it doesn't introduce a breaking change, which I don't think it would? The interface would just need to include things like header(), confg(), body() right? Thing is, body is optional completely.

Wish you all the best too!!

[bobmulder]

@Sammyjo20, I changed the RequestValidator and InstanceOfCheck, so they use a PendingRequest now.

Nice thing I realised when working on the InstanceOfCheck, whenever it needs the original request, it's just accessible:

    public function valid(): bool
    {
        return $this->actual->getRequest() instanceof $this->expected;
    }

I also moved the Validators namespace to Testing as you suggested before.

Next, I'm going to work out some more checks!

[Sammyjo20]

Just marking this as draft as it's still WIP - keep up the great work @bobmulder !

[bobmulder]

Hi Sammy, I lost track on this one but I'm motivated to finish the PR since I think it's pretty useful. Can we together work out a todo list of what needs to be done to get this ready to merge? I'd appreciate and finish the pending work.

[Sammyjo20]

Hey @bobmulder Happy New Year. That's okay, these things happen. I'm not able to spend a huge amount of time on Saloon at the moment but over the next few weeks I will try to write up a list of things to do. I've also been thinking of ways to improve the testing in Saloon so I can include those changes too if you like?

[Sammyjo20]

Hey @bobmulder how are you doing? Do you have any further plans for this PR or shall we revisit it in the future? ❤️