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.

Sign up for GitHub

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

Jump to bottom

Fix integration tests #10

Open
wants to merge 6 commits into
base: integration-tests-issues
Choose a base branch
from
Open

Fix integration tests #10

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f37b711
Fix integration tests
snapshotpl Jul 13, 2021
0c32a43
Skip 500 errors
snapshotpl Jul 13, 2021
ef7ece7
Rollback date
snapshotpl Jul 13, 2021
db4eb5e
Fix DTO
snapshotpl Jul 13, 2021
3cb0bbb
Restore Error model
snapshotpl Jul 13, 2021
036b776
Post CR improvments and remove skip 500 responses
snapshotpl Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Model/Organization.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,5 @@ class Organization extends DataTransferObject

public ?Address $invoice_address;

public Person $contact_person;
public ?Person $contact_person;
}
4 changes: 2 additions & 2 deletions src/Response/HttpResponseProcessor/AbstractJsonContentProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ abstract class AbstractJsonContentProcessor implements ProcessorInterface
public function run(MethodInterface $method, ResponseInterface $httpResponse): ?Response
{
if (in_array($httpResponse->getStatusCode(), $this->getHttpStatusCodes())) {
$contentType = explode('; ', $httpResponse->getHeaderLine('Content-Type'));
$mediaType = reset($contentType);
$contentType = explode(';', $httpResponse->getHeaderLine('Content-Type'));
$mediaType = trim(reset($contentType));
if ($mediaType === 'application/json') {
$httpResponse->getBody()->rewind();
$data = json_decode($httpResponse->getBody()->getContents(), true);
Expand Down
9 changes: 6 additions & 3 deletions src/Response/HttpResponseProcessor/ErrorProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,16 @@ protected function getPayload(
// This handles inconsistency in ShipX API. Some errors have HTTP status code 200 and error data in body.
if (isset($data['status']) && $data['status'] !== 200 && isset($data['key']) && isset($data['error'])) {
return new Error([
'status' => $data['status'],
'error' => $data['key'],
'message' => $data['error']
'status' => $data['status'] ?? $httpResponse->getStatusCode(),
'error' => $data['key'] ?? '',
'message' => $data['error'] ?? '',
Copy link
Owner

Choose a reason for hiding this comment

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

@snapshotpl This three lines of changes are unnecessary. Existence of status, key and error is already checked in if condition.

Copy link
Owner

Choose a reason for hiding this comment

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

@snapshotpl These three lines of changes are unnecessary. Existence of status, error and message is already checked in if condition.

]);
}
return null;
}
$data['status'] ??= $httpResponse->getStatusCode();
Copy link
Owner

Choose a reason for hiding this comment

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

@snapshotpl Wow! I didn't know ??= operator 👍.

$data['error'] = $data['key'] ?? '';
$data['message'] = $data['error'] ?? '';
Copy link
Owner

Choose a reason for hiding this comment

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

@snapshotpl The default (documented) error response has status, error, message and (optional) details. Please fill in message with error and error with key only if message doesn't exist.

I know the whole thing is stupid, but inconsistency of Inpost API is even more stupid 👎.

return new Error($data);
}
}
5 changes: 5 additions & 0 deletions tests/Integration/Client/TestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,12 @@ protected function assertSuccessWithFile(Response $response, DataTransferObject
{
if (!$response->getSuccess()) {
$this->debug(print_r($response->getPayload()->toArray(), true));

if ($response->getPayload()->status === 500) {
Copy link
Owner

Choose a reason for hiding this comment

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

@snapshotpl In what cases did you encounter 500 errors?

Copy link
Author

Choose a reason for hiding this comment

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

shipx-php-sdk/tests/Integration/Client/ReportResourceTest.php

Line 18 in cbdd3ab

public function testGetCodSuccessfulCall(): void

Copy link
Owner

Choose a reason for hiding this comment

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

Do you encounter this 500 error every time you perform a request? I have never had issues with this one, nor do I have it now...

I would rather not skip test because of 500 error. Such mechanism might hide some real issues in the future.

$this->markTestSkipped('500 error: '. $response->getPayload()->message);
}
}

$this->assertTrue($response->getSuccess());
$this->assertInstanceOf(BinaryContent::class, $payload);
/** @var BinaryContent $payload */
Expand Down