refactor tests #8
Conversation
tests/ResponseTest.php
Outdated
| $response = $this->service->respondNotFound('Ouch!'); | ||
| self::assertInstanceOf(JsonResponse::class, $response); | ||
| self::assertEquals(Response::HTTP_NOT_FOUND, $response->getStatusCode()); | ||
| self::assertJsonStringEqualsJsonString('{"error":"Ouch!"}', $response->getContent()); | ||
| self::assertEquals(['error' => 'Ouch!'], $response->getData(true)); | ||
| $expected_response['status_code'] = Response::HTTP_NOT_FOUND; | ||
| $expected_response['json'] = ['error' => 'Ouch!']; | ||
| $this->testResponse($expected_response, $response); |
There was a problem hiding this comment.
If you look closely then you can see that this pattern repeats in almost all tests. This is what we want to fix, remove repetitions.
Whenever anything repeats like that it may be a good idea to pull it out into something, either function, method or some type of other "thing". How do we know how to write that "thing"? Look at what is different.
We call the service method A with some argument B, then check for value C of status_code and value D of json. Then we call testResponse method with response and expected_response we built from A, B, C and D. This happens in every repetition, with few exceptions where we don't check the status.
So, pulling out all that code to some method that does what we need based on A, B, C, D is the first step. Well, you can ignore pulling out the method A, that may add too much complexity.
Second step would be to use things built into PHPUnit, namely data providers. Please find PHPUnit documentation and read the chapter about them. If you feel OK with it then maybe try to make needed changes to use them. I recommend that if you do please do name the data sets.
If anything is not clear please ask, I will gladly answer questions.
There was a problem hiding this comment.
Yep, exactly what I looking for. The current tests were written in ~15 minutes so I could release the package with some confidence. Data providers and a single testResponse (or similar method) is the approach I was going to use 👍
There was a problem hiding this comment.
Actually, I'd say 80% of these tests can be moved to a single method with a data provider.
There was a problem hiding this comment.
Note your changes cause tests to fail :) The testResponse method is picked up by PHPUnit.
There was a problem hiding this comment.
Thank you for the feedback, i did look into PHPUnit docs and read about data providers and used them to squeeze all the tests in a single testResponse method however i see that @f9web has already commited similar changes.
There was a problem hiding this comment.
No problem, I have released this now, see https://github.com/f9webltd/laravel-api-response-helpers/releases/tag/1.4.1. I've added yourself and @JacekAndrzejewski as contributors 👍
|
@rkhan06 I've just released |
Fix tests - rename `testResponse` method as it is picked up by PHPUnit by default
Refactor tests into a single method using a data provider
|
I've gone ahead an refactored tests into a single method using data provider. Is there any feedback before I go ahead and merge this in? |
Fix read me link and typo in Laravel 7 workflow
|
fixes #1 |
tried to refactor tests