-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
handle Guzzle ClientException #324
handle Guzzle ClientException #324
Conversation
Codecov Report
@@ Coverage Diff @@
## master #324 +/- ##
=========================================
Coverage 99.68% 99.68%
- Complexity 351 352 +1
=========================================
Files 20 20
Lines 940 942 +2
=========================================
+ Hits 937 939 +2
Misses 3 3
Continue to review full report at Codecov.
|
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.
Hi @individual-it, thank you for reporting the issue and working on a fix. The core changes look ok to me. However, I have some comments on changes related to the test.
The expectation from getStatusCode
is not changed, so we don't need to touch that part. What we can do here is, mock the guzzle ClientException and add expectations for it.
Something like this would make more sense:
diff --git a/tests/Tus/ClientTest.php b/tests/Tus/ClientTest.php
index 38e4244..3036405 100644
--- a/tests/Tus/ClientTest.php
+++ b/tests/Tus/ClientTest.php
@@ -2,7 +2,6 @@
namespace TusPhp\Test\Tus;
-use GuzzleHttp\Psr7\Request;
use Mockery as m;
use GuzzleHttp\Client;
use TusPhp\Cache\FileStore;
@@ -1350,11 +1349,10 @@ class ClientTest extends TestCase
$checksum = hash_file('sha256', $filePath);
$guzzleMock = m::mock(Client::class);
$responseMock = m::mock(Response::class);
- $requestMock = m::mock(Request::class);
$responseMock
->shouldReceive('getStatusCode')
- ->twice()
+ ->once()
->andReturn(400);
$this->tusClientMock
@@ -1369,6 +1367,13 @@ class ClientTest extends TestCase
$this->tusClientMock->file($filePath, $fileName);
+ $clientExceptionMock = m::mock(ClientException::class);
+
+ $clientExceptionMock
+ ->shouldReceive('getResponse')
+ ->once()
+ ->andReturn($responseMock);
+
$guzzleMock
->shouldReceive('post')
->once()
@@ -1380,13 +1385,7 @@ class ClientTest extends TestCase
'Upload-Metadata' => 'filename ' . base64_encode($fileName),
],
])
- ->andThrow(
- new ClientException(
- 'Client error: `POST files` resulted in a `400 Bad Request` response',
- $requestMock,
- $responseMock
- )
- );
+ ->andThrow($clientExceptionMock);
$this->tusClientMock->create($key);
}
@ankitpokhrel thank you for your review. I've adjusted the tests according to your suggestions |
Thank you @individual-it 🎉 |
@ankitpokhrel could you trigger a new release, so that I can easily use it with composer? |
@individual-it I will do it in few hours. |
Released v2.1.1 |
in case of an 4xx error Guzzle throws a ClientException and the response is inside of that
fixes #323