Maximal test coverage for "ClientInterface" implementing classes

[aik099]

Problems solved:

• check check for non-array $data parameter during GET request value in CurlClient class was made after it was passed into http_build_query function call and that caused a notice
• no check was made for $data being an array during GET request in PHPClient
• the PHPClient wasn't sending correct Content-Type header, when making GET request
• support for DELETE calls in PHPClient
• file uploads via CurlClient were not working in PHP < 5.5 and HHVM (regression introduced in 96ab84e)
• file uploads via CurlClient were not working in PHP > 7.0 (regression introduced in 048d96d)
• the PHPClient was throwing different exceptions, then CurlClient for same HTTP responses

Problems not solved:

• PHPClient test that is getting HTTP 201 and HTTP 204 responses on HHVM 3.6.6 (Travis CI version) is failing for unknown reason. Locally I've tried on HHVM 3.13.1 and it works. I'm guessing this is HHVM bug.
• Not sure how to write test that would emulate communication error.
• Not sure how to write where test was no communication error, but somehow response is NULL.

Also related to #100.

[aik099]

I've decided to add some more tests in here, so please don't merge yet.

[aik099]

@jpastoor , ready for review.

[jpastoor]

Cool I'll check it out this weekend

[jpastoor]

I think it would have been nicer to rebase & land #76 instead of putting it in your own code. (Especially since the rebase was needed due to a code style change you did yourself)

[aik099]

That thought have crossed my mind, but I decided not to take that patch considering much more actions involved from my side.

Since you've asked I've created #115 now and once merged I can rebase this PR on top of it.

[jpastoor]

Cool

[aik099]

Rebased and pushed.

[jpastoor]

With this change we would introduce the need for a webserver to run the unit tests for this library. This makes it harder for potential contributers to run the unit test suite.
Also the actual curl calls do not need testing, we can safely assume they work.

I propose either
a. restructuring the code a bit so you can test the options array that goes into curl, and leave the actual sending untested.
b. move the tests that require a webserver to a seperate test suite, which can run independently of the rest
c. (not sure how feasible) setup a slim webserver in the phpunit bootstrap code
(suggestions in order of my personal preference)

[jpastoor]

Not sure how to write test that would emulate communication error. Not sure how to write where test was no communication error, but somehow response is NULL.

In this case with option a. of above that would be mocking the actual curl_exec call so you can influence the outcome. You would wrap that call in an injectable class (pseudo code incoming)

class CurlWrapper public function exec($options) { $curl = curl_init(); curl_setopt_array($curl, $options); return curl_exec($curl); } }

and inject that in the CurlClient.

You can ignore those lines explicitly from the coverage report.
For your different tests you can put assertions on the $options input and override the return content to check CurlClient behaviour.

[aik099]

With this change we would introduce the need for a webserver to run the unit tests for this library.

Ha, you've missed the whole point. Tests, that require Web Server only run, when REPO_URL environment variable is present and it's not present by default. This way:

• users who don't define REPO_URL in their phpunit.xml will just see all client class tests as skipped
• users who define REPO_URL will do it on purpose and by doing so will confirm, that they have running web server

In the CONTRIBUTING.md I've mentioned REPO_URL as one of setup steps, but I should probably mention, that this step is optional.

[aik099]

In this case with option a. of above that would be mocking the actual curl_exec call so you can influence the outcome. You would wrap that call in an injectable class (pseudo code incoming)

Don't be CURL-oriented. We need solution that will cover all existing and further client classes. Somebody might want to create GuzzleClient class someday maybe and we need to see that it actually works, not that it works on our mocked curl_exec.

[jpastoor]

Woops that is pretty sweet then Alex. Did not see that they were already optional indeed.

[aik099]

Woops that is pretty sweet then Alex. Did not see that they were already optional indeed.

So please next time try to run test suite before trying to suggest workaround for non-existing problem. 😉

[jpastoor]

Still think it would be a nice abstraction the one I suggested.

Makes for cleaner tests and easier input/output control. (You would not need to do it for guzzle since that has the abstraction build in already)

[aik099]

Still think it would be a nice abstraction the one I suggested.

As I've written above it's curl-oriented and we can't use it to test any client.

Makes for cleaner tests and easier input/output control. (You would not need to do it for guzzle since that has the abstraction build in already)

We have an abstraction too: the ClientInterface. You can't abstract the abstraction by making architectural changes used just to make it more testable. Sometimes, and I believe this is the time, it's much more easier to stay with an integration test, rather to try unit test it and make necessary code changes to make it unit testable.

[aik099]

I've updated this part in CONTRIBUTING.md to stress, that webserver presence is optional.