-
Notifications
You must be signed in to change notification settings - Fork 64
API-599: replace integration tests by end-to-end tests #132
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
Conversation
ac2c5d5 to
0621a09
Compare
| language: php | ||
|
|
||
| php: | ||
| - 7.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5.6
7.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
tests/Api/ApiTestCase.php
Outdated
| /** | ||
| * @return StreamFactory | ||
| */ | ||
| protected function getStreamFactory() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is it used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In UpsertListProductTest sur le test test_upsert_list_from_stream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed to share with every tests in this case.
And if it's function shared across several test, it's a good candidate about using a trait.
| 'locale' => null, | ||
| ]); | ||
|
|
||
| $this->assertSame('f/b/0/6/fb068ccc9e3c5609d73c28d852812ba5faeeab28_akeneo.png', $response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert::assertSame
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check in the history of the response stack (take a look at the library documentation) what if the body of the request what what we expects.
For now, you are just checking the URI call.
tests/Api/CreateProductTest.php
Outdated
| $api = $this->createClient()->getProductApi(); | ||
| $response = $api->create('new_shoes', $this->newProduct()); | ||
|
|
||
| $this->assertSame(201, $response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same (almost evrywhere): check the body of the request. You can do a function in ApiTestCase.
tests/Api/ListPerPageProductTest.php
Outdated
| $api = $this->createClient()->getProductApi(); | ||
| $firstPage = $api->listPerPage(10, true, []); | ||
|
|
||
| $this->assertInstanceOf(PageInterface::class, $firstPage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert::
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed everywere
20ef38e to
579ced8
Compare
…entation to test with docker
9061ee0 to
6221347
Compare
5b31354 to
b2083d4
Compare
0b85c57 to
5d3cba1
Compare
1ac139f to
cf0b8cb
Compare
7755bcc to
e5b639e
Compare
…entation to test with docker
74d9d02 to
1aacf98
Compare
composer.json
Outdated
| "php-http/multipart-stream-builder": "^1.0", | ||
| "php-http/client-implementation": "^1.0" | ||
| "php-http/client-implementation": "^1.0", | ||
| "php-http/guzzle6-adapter": "^1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both in "require" and "require-dev" ? I think it shouldn't be in "require"
2dc0d86 to
97cca32
Compare
|
@LaurentPetard I remove both, we add it by composer require in the doc so I don't have to add it in composer.json |
| $response = $api->create($mediaFile, $productInfos); | ||
|
|
||
| Assert::assertSame($this->server->getLastRequest()->jsonSerialize()[RequestInfo::JSON_KEY_POST]['product'], json_encode($productInfos)); | ||
| Assert::assertSame($this->server->getLastRequest()->jsonSerialize()[RequestInfo::JSON_KEY_REQUEST_URI], '/'. ProductMediaFileApi::MEDIA_FILES_URI); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useless line
phpunit.xml.dist
Outdated
| convertNoticesToExceptions="true" | ||
| convertWarningsToExceptions="true" | ||
| processIsolation="true" | ||
| processIsolation="false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can keep it at true (maybe my commit which did that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I change that
| const LOWER_THAN_ON_ALL_LOCALES = 'LOWER THAN ON ALL LOCALES'; | ||
| const LOWER_OR_EQUALS_THAN_ON_ALL_LOCALES = 'LOWER OR EQUALS THAN ON ALL LOCALES'; | ||
| const EMPTY = 'EMPTY'; | ||
| const IS_EMPTY = 'EMPTY'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is a BC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the end, we will have to change it because it is not supported by php 5.6.... it's a kind of bug
tests/Api/CreateProductTest.php
Outdated
| $api = $this->createClient()->getProductApi(); | ||
| $response = $api->create('new_shoes', $this->newProduct()); | ||
|
|
||
| Assert::assertSame($this->server->getLastRequest()->jsonSerialize()[RequestInfo::JSON_KEY_INPUT], json_encode(array_merge($this->newProduct(), ["identifier" => "new_shoes"]))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of it.
You are doing logic inside your test by doing array_merge. Avoid that.
Put directly what you are expecting. Copy past in test is not a problem.
(it means that the function newProduct is usless)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| { | ||
| public function test_download_media_file() | ||
| { | ||
| $expectedMediaFile = realpath(__DIR__ . '/../fixtures/akeneo.png'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FilePath, FIle is for generally for a ressource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| Assert::assertSame($this->server->getServerRoot() . '/api/rest/v1/products?page=2&with_count=true&pagination_type=page&limit=10', $firstPage->getNextLink()); | ||
| Assert::assertEquals(count($firstPage->getItems()), 10); | ||
|
|
||
| $secondPage = $firstPage->getNextPage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert the last request for the query parametesrs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already present a few line before : Assert::assertSame($this->server->getLastRequest()->jsonSerialize()[RequestInfo::JSON_KEY_GET], ['limit' => '10', 'with_count' => 'true']);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but nextPage does another new request. It's a cursor under the hood. So you have to check that this new call is done with good query parameters
|
Almost GTM: please revert the BC break |
6f14728 to
0e4cf53
Compare
2f39841 to
e2fcf8f
Compare
| const LOWER_THAN_ON_ALL_LOCALES = 'LOWER THAN ON ALL LOCALES'; | ||
| const LOWER_OR_EQUALS_THAN_ON_ALL_LOCALES = 'LOWER OR EQUALS THAN ON ALL LOCALES'; | ||
| const EMPTY = 'EMPTY'; | ||
| const IS_EMPTY = 'EMPTY'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the end, we will have to change it because it is not supported by php 5.6.... it's a kind of bug
Replace all integration tests by end-to-end tests using "donatj/mock-webserver".