joesaunderson / sportmonks-soccer

PHP API Client for Sportmonks Soccer API
MIT License
22 stars 14 forks source link

Unit tests and a base for easier testing endpoints #10

Open davidribeiro opened 4 years ago

davidribeiro commented 4 years ago

This is to fix #9 (or at least, create a base so that it's easier to test the "endpoint" classes) Added:

joesaunderson commented 4 years ago

Looks good, so the JSON file must match the name of the parameter? Am i reading that correctly?

davidribeiro commented 4 years ago

Yeah, pretty much it the same. Instead of having an API where you call /resource/id, you have in your mocks a id.json in the resource folder. That way you don't need to waste much time creating mocks for the responses. Just my way of thinking so that you dont need much to implement tests for the endpoints

joesaunderson commented 4 years ago

I’m not sure if that would work with all of the resources, and some allow additional params (which I think are parsed into the query string).

We could potentially do it differently, making the filename protected, then for each test case, override the filename?

davidribeiro commented 4 years ago

I understand, that's why I'm just saying that this is just a base endpoint test case. I'm not very familiar with the sportmonks API but in any case, we could just rearrange the way we store the mocks to support also query parameters. That way we just need to modify the callback in the Endpoint test case class to map the $url variable to the correct file in the mocks folder.

The way I see it, the only thing we should test in the endpoint classes is the $url variable that is being passed to the request method. What happens in the request method doesn't belong to the project responsibilities, that's why the project uses symfony's package to handle the call to the API. What do you think?

tarlepp commented 4 years ago

Basically you really don't want to make real requests to 3rd party API's within your tests - that makes no sense. eg. if that 3rd party service is down your tests will fail - and you cannot control that. Another point is that usually those 3rd services have somekind of rate limit within those API requests, so you will hit those limits quite soon too. And basically if/when you're using those 3rd api's in your tests you're testing that 3rd party api, not your application.

And how to fix this? There is PHP-VCR library to mock those 3rd party API calls totally, which basically solves all the problems that I described earlier.

joesaunderson commented 4 years ago

I think the OP is using HTTP client to mock the requests?

davidribeiro commented 4 years ago

I'm actually using Symfony\Component\HttpClient\MockHttpClient, which does no http calls whatsoever. This class is already in the symfony package the project was already using and it's used for testing purposes, to mock the responses without using any external dependency.

I just took a quick look at the PHP-VCR package and it seems like it's doing pretty much the same thing I've done. I wouldn't use it just because I try to use the less dependencies I can, and I believe that having the mocks coded by yourself makes you more in control of your own project and it's also a great way for you to develop your testing skills. I'm not saying the package is bad, I just believe it's an overkill for a simple project.

tarlepp commented 4 years ago

And in my opinion it's not "secure" to mock the client itself - what if the client itself changes some basic behaviour (and yes I know that Symfony has BC promise, but that won't still cover that case) on that component itself. That is the main reason I mentioned that PHP-VCR library.

tarlepp commented 4 years ago

also note that those mocks are created automatically on first request where you're trying to use those - so there isn't really manual work at all.

davidribeiro commented 4 years ago

When you use dependencies, you're always allowing some kind of upgrade where the dependency changes and you're left with a broken project, that's why I try to use the least amount of dependencies possible. I've never used the PHP-VCR so I'm not saying it's bad or good, I'm just saying that it's not bringing anything better than the solution I provided does. It's also a dependency, so it's also "insecure"

tarlepp commented 4 years ago

Sure, I'm just saying my point of view of this - so seems like we just need to agree to disagree with this - and that is just fine.