Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit tests and a base for easier testing endpoints #10

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Unit tests and a base for easier testing endpoints #10

wants to merge 5 commits into from

Conversation

davidribeiro
Copy link

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

  • phpunit on composer's dev dependencies
  • Implemented EndpointTestCase that uses Symfony's MockHttpClient so that it's easier to test endpoint classes
  • Also created tests for SoccerApi and SoccerApiHelper classes
    Any feedback is appreciated !

@davidribeiro davidribeiro mentioned this pull request Oct 2, 2019
@joesaunderson
Copy link
Owner

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

@davidribeiro
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Contributor

tarlepp commented Oct 6, 2019

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
Copy link
Owner

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

@davidribeiro
Copy link
Author

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
Copy link
Contributor

tarlepp commented Oct 6, 2019

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
Copy link
Contributor

tarlepp commented Oct 6, 2019

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
Copy link
Author

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
Copy link
Contributor

tarlepp commented Oct 6, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Unit Tests
3 participants