[API] Initial API tests #3701

lchrusciel · 2015-12-10T15:30:07Z

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Fixed tickets	#3468
License	MIT
Doc PR	#371

I want to introduce initial tests for Sylius API. The previous ApiTestCase was removed from Sylius and after small refactoring and a few improvements we prepared a stand-alone library Lakion ApiTestCase. I added this library to composer and wrote some sample test for Shipping Categories. I want to test all available endpoints so feel free to help me with this task.

Update:
NOTE: Because of depending on PHP to string, which has a class String the tests will fail on PHP7. We are waitng to merge PR which will resolve this issue.

Update2:
NOTE: PHP to string was fixed and it supports PHP7. But because we are still waiting for the official release temporary I added "minimum-stability": "dev" to composer. Do not merge it yet.

Update3:
NOTE: As far as we are good with depending on PHP-Matcher dev explicitly its ready for merge. At the end, ApiTestCase doesn't have a stable version to.

pjedrzejewski · 2015-12-10T17:50:03Z

src/Sylius/Bundle/CoreBundle/Tests/Controller/ShippingCategoryControllerTest.php

+/**
+ * @author Łukasz Chruściel <lukasz.chrusciel@lakion.com>
+ */
+class ShippingCategoryControllerTest extends JsonApiTestCase


I am wondering if we should name tests like this. The ShippingCategoryController class does not exist and people are used to the fact that there is usually Xyz class and XyzTest. Something that sounds less like a class will be less confusing I hope.

ShippingCategoryApiTest vs ShippingCategoryControllerTest

WDYT?

I'm for ShippingCategoryApiTest to minimalize confusion.

ShippingCategoryApiTest is fine for me.

pjedrzejewski · 2015-12-10T18:01:54Z

Nice work Łukasz! Let's hear more comments.

michalmarcinkowski · 2015-12-11T09:41:17Z

src/Sylius/Bundle/CoreBundle/Tests/Controller/ShippingCategoryControllerTest.php

+        $this->assertResponse($response, 'shipping_category/create_validation_fail', 400);
+    }
+
+    public function testCreateShippingCategoryResponse()


I'm wondering if we should suffix all methods with Response. Isn't it enough to call it testCreateShippingCategory?

The question is: do we test logic or received response?

We are testing response contents/headers only so I am fine with leaving that suffix.

IMHO we test logic and we check if it's correct by comparing received responses.

lchrusciel · 2015-12-11T10:54:08Z

@bendavies Thanks for advices! I have to update ApiTestCase README to, to point out these features ;)

bendavies · 2015-12-11T11:09:38Z

phpunit.xml.dist

+    </testsuites>
+
+    <php>
+        <server name="KERNEL_CLASS_PATH" value="/app/TestKernel.php" />


missing KERNEL_DIR?

ApiTestCase allows you to use Kernel class directly. We need it, because we have two separated Kernels. The problem is with version of ApiTestCase on travis.

pjedrzejewski · 2015-12-18T10:04:54Z

phpunit.xml.dist

+         bootstrap="app/bootstrap.php.cache"
+>
+    <testsuites>
+        <testsuite name="Project Test Suite">


Sylius Test Suite

pamil · 2015-12-18T10:17:01Z

@Arn0d neither WebTestCase nor our Behat with Symfony2Extension (without JS) send a real HTTP request as both of them use Symfony\Bundle\FrameworkBundle\Client under the hood

pamil · 2015-12-18T10:20:14Z

phpunit.xml.dist

+>
+    <testsuites>
+        <testsuite name="Project Test Suite">
+            <directory>src/*/*Bundle/Tests</directory>


Do we have any directory that matches this pattern? Shouldn't it be src/*/Component/*/Tests as the component equivalent of the bundle one line below?

I'd remove this. Components don't have PHPUnit test and won't have them anytime soon I guess.

arnolanglade · 2015-12-18T10:29:16Z

@pamil I know but it is enough for web case (get and post) when you use put method and you want to update a file, it is another story ;). PHP does not decode HTTP header... I just wanted to warn you but anyway, even if I prefer behat I prefer you test the API with any solution because :

pamil · 2015-12-18T10:37:07Z

@Arn0d didn't know about that, thanks! Does it happen only on PUT request, but not on POST ones? Last week I was uploading image in some scenario on while creating taxonomy (POST) and it worked like a charm.

michalmarcinkowski · 2015-12-18T10:41:24Z

src/Sylius/Bundle/ApiBundle/Resources/config/routing/shipping_category.yml

@@ -7,6 +7,7 @@ sylius_api_shipping_category_index:
    defaults:
        _controller: sylius.controller.shipping_category:indexAction
        _sylius:
+            serialization_groups: [Default]


Is it needed? Isn't Default a default serialization group?

Right now:

https://github.com/Sylius/SyliusResourceBundle/blob/master/Controller/Configuration.php#L369

I don't know if JMS requires to specify Default explicitly as it won't add it automatically if you pass an empty array - not sure. So I think we should add 'Default' to the Configuration::getSerializationGroups() method which I linked above.

Shouldn't it be done in a separate PR?

arnolanglade · 2015-12-18T10:43:03Z

@pamil Just with PUT and PATCH I did not test the latest version of PHP.

michalmarcinkowski · 2015-12-18T10:44:21Z

src/Sylius/Bundle/CoreBundle/Tests/Controller/ShippingCategoryApiTest.php

+        ]);
+
+        $response = $this->client->getResponse();
+        $this->assertResponse($response, 'shipping_category/create_validation_fail', 400);


Shouldn't we use constant for codes? E.g. Response::HTTP_BAD_REQUEST here.

Yes, we should ;)

pjedrzejewski · 2015-12-18T12:24:14Z

Please change the payload formatting according to my sample PR if you agree and update ApiTestCase version. And we are good to go! Good job Łukasz!

pjedrzejewski · 2015-12-24T23:11:05Z

@lchrusciel Please rebase. :)

lchrusciel · 2015-12-28T00:09:48Z

@pjedrzejewski done! I have done a little refactoring also, cuz of changes in ApiTestCase and feature added by @bendavies.

[API] Initial API tests

pjedrzejewski · 2015-12-28T00:30:50Z

Awesome work, thank you Łukasz! 👍

pjedrzejewski · 2015-12-28T23:37:42Z

src/Sylius/Bundle/CoreBundle/Tests/Controller/ShippingCategoryApiTest.php

+        $this->client->request('PATCH', '/api/shipping-categories/'.$shippingCategories['shipping_category_1']->getId(), [], [], [
+            'HTTP_Authorization' => 'Bearer SampleTokenNjZkNjY2MDEwMTAzMDkxMGE0OTlhYzU3NzYyMTE0ZGQ3ODcyMDAwM2EwMDZjNDI5NDlhMDdlMQ',
+            'CONTENT_TYPE' => 'application/json',
+        ], '{"name": "Light", "description": "Light weight items"}');


Would be great to update these to use the format I suggested above.

pjedrzejewski · 2015-12-28T23:38:15Z

@lchrusciel Please have a look at my comment above and also... seems like we missed test for delete? /cc @michalmarcinkowski

lchrusciel force-pushed the lets-test-some-apis branch from 5602d4c to b892ce8 Compare December 10, 2015 15:37

pjedrzejewski reviewed Dec 10, 2015
View reviewed changes

pjedrzejewski added the New Feature label Dec 10, 2015

michalmarcinkowski reviewed Dec 11, 2015
View reviewed changes

lchrusciel force-pushed the lets-test-some-apis branch 2 times, most recently from 2436168 to 66cc1f6 Compare December 11, 2015 10:48

lchrusciel force-pushed the lets-test-some-apis branch 3 times, most recently from 3074947 to a4c2b96 Compare December 11, 2015 11:06

bendavies reviewed Dec 11, 2015
View reviewed changes

pjedrzejewski mentioned this pull request Dec 11, 2015

Php7 support coduo/php-to-string#3

Closed

lchrusciel force-pushed the lets-test-some-apis branch 2 times, most recently from a4c2b96 to ef68913 Compare December 15, 2015 13:35

lchrusciel force-pushed the lets-test-some-apis branch from 9ecf8bd to 37f385e Compare December 18, 2015 09:58

pjedrzejewski reviewed Dec 18, 2015
View reviewed changes

phpunit.xml.dist

bootstrap="app/bootstrap.php.cache"

>

<testsuites>

<testsuite name="Project Test Suite">

Copy link

Member

pjedrzejewski Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sylius Test Suite

pamil reviewed Dec 18, 2015
View reviewed changes

michalmarcinkowski reviewed Dec 18, 2015
View reviewed changes

lchrusciel force-pushed the lets-test-some-apis branch from e200cce to acf044d Compare December 18, 2015 14:52

pjedrzejewski mentioned this pull request Dec 21, 2015

[ResourceBundle] New ResourceController #3762

Merged

pjedrzejewski removed the Awaiting Review label Dec 24, 2015

lchrusciel added 4 commits December 28, 2015 01:07

[API] Add ApiTestCase to composer

32830ea

[API] ApiTestCase configuration

ee087c1

[API] Shipping Category API tests

03693e4

[API] Tests refactoring

e7b8b26

lchrusciel force-pushed the lets-test-some-apis branch from acf044d to e7b8b26 Compare December 28, 2015 00:08

pjedrzejewski pushed a commit that referenced this pull request Dec 28, 2015

Merge pull request #3701 from lchrusciel/lets-test-some-apis

767f79f

[API] Initial API tests

pjedrzejewski merged commit 767f79f into Sylius:master Dec 28, 2015

pjedrzejewski reviewed Dec 28, 2015
View reviewed changes

lchrusciel deleted the lets-test-some-apis branch February 1, 2016 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API] Initial API tests #3701

[API] Initial API tests #3701

lchrusciel commented Dec 10, 2015

pjedrzejewski Dec 10, 2015

michalmarcinkowski Dec 11, 2015

lchrusciel Dec 11, 2015

pjedrzejewski commented Dec 10, 2015

michalmarcinkowski Dec 11, 2015

lchrusciel Dec 11, 2015

pjedrzejewski Dec 11, 2015

pamil Dec 11, 2015

lchrusciel commented Dec 11, 2015

bendavies Dec 11, 2015

lchrusciel Dec 11, 2015

pjedrzejewski Dec 18, 2015

pamil commented Dec 18, 2015

pamil Dec 18, 2015

pjedrzejewski Dec 18, 2015

arnolanglade commented Dec 18, 2015

pamil commented Dec 18, 2015

michalmarcinkowski Dec 18, 2015

pjedrzejewski Dec 18, 2015

lchrusciel Dec 18, 2015

arnolanglade commented Dec 18, 2015

michalmarcinkowski Dec 18, 2015

lchrusciel Dec 18, 2015

pjedrzejewski Dec 18, 2015

pjedrzejewski commented Dec 18, 2015

pjedrzejewski commented Dec 24, 2015

lchrusciel commented Dec 28, 2015

pjedrzejewski commented Dec 28, 2015

pjedrzejewski Dec 28, 2015

pjedrzejewski commented Dec 28, 2015

[API] Initial API tests #3701

[API] Initial API tests #3701

Conversation

lchrusciel commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjedrzejewski commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchrusciel commented Dec 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pamil commented Dec 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnolanglade commented Dec 18, 2015

pamil commented Dec 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnolanglade commented Dec 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjedrzejewski commented Dec 18, 2015

pjedrzejewski commented Dec 24, 2015

lchrusciel commented Dec 28, 2015

pjedrzejewski commented Dec 28, 2015

Choose a reason for hiding this comment

pjedrzejewski commented Dec 28, 2015