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.

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

Fix integration tests #10

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

Conversation

snapshotpl
Copy link

No description provided.

@snapshotpl
Copy link
Author

I have fixed all possible errors and failures in integration test. Last thing I cant fix is enable courier in sandbox account, so 5 tests didn't pass.

@michalbiarda
Copy link
Owner

@snapshotpl You need to write to InPost support and ask them to enable courier on your Sandbox account. That's how I did.

Thanks for the fixes, I'll review them in free time. The Sandbox is unfortunately pretty unstable 😔.

Copy link
Owner

@michalbiarda michalbiarda left a comment

Choose a reason for hiding this comment

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

Could you please answer my question in review and refactor one of your fixes? I'll try to check later the last issue.

@@ -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.

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.

src/Model/Error.php Outdated Show resolved Hide resolved
'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.

'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 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 👍.

]);
}
return null;
}
$data['status'] ??= $httpResponse->getStatusCode();
$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 👎.

@@ -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.

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.

@michalbiarda
Copy link
Owner

@snapshotpl When you're done with your work, please create a merge request to integration-tests-issues branch. I'll then handle the last issues and then merge it to master. Thanks!

@snapshotpl snapshotpl changed the base branch from master to integration-tests-issues July 14, 2021 13:27
@snapshotpl
Copy link
Author

After enable courier on my sandbox account only one test fails:

1) MB\ShipXSDK\Test\Integration\Client\ShipmentResourceTest::testSuccessfulSimpleFlow
Failed asserting that false is true.

/shipx-php-sdk/tests/Integration/Client/TestCase.php:54
/shipx-php-sdk/tests/Integration/Client/ShipmentResourceTest.php:393
/shipx-php-sdk/tests/Integration/Client/ShipmentResourceTest.php:109

@snapshotpl
Copy link
Author

@michalbiarda ping :-)

@snapshotpl
Copy link
Author

@michalbiarda Can I help somehow with this release?

@michalbiarda
Copy link
Owner

@snapshotpl Unfortunately I have to abandon this project due to lack of time for development / maintenance :-(. I'm sorry.

@snapshotpl
Copy link
Author

@michalbiarda understand. Can I take this code and move forward my own namespace?

@michalbiarda
Copy link
Owner

@snapshotpl Please do whatever you want with the code. Just put a link to this repo in your readme and mention me there. Hope you'll have time to finish it and maintain it. There's not much left I think :-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants