removal of Throwables through _sendRequest from get(), post(), patch(), put(), delete, head(), & options()#14193
removal of Throwables through _sendRequest from get(), post(), patch(), put(), delete, head(), & options()#14193travisfont wants to merge 8 commits intocakephp:masterfrom travisfont:enhancement/integration_test_trait_exception_removal
Conversation
…), put(), delete, head(), & options()
| * @param string|array $url The URL to request. | ||
| * | ||
| * @return void | ||
| * @throws \PHPUnit\Framework\Error\Error|\Throwable |
There was a problem hiding this comment.
Arent those true if you disable error handler middleware?
There was a problem hiding this comment.
Like if any business logic or in the controller throws exceptions, and we disable the error catching then this would still be true, right?
There was a problem hiding this comment.
These are still not answered here. I still think those should be kept as those can through exceptions if error handler middleware is disabled.
|
@tfont are you going to follow up on this? |
@othercorey Yes, I can update this. |
…Error() if existing.
|
@othercorey @markstory |
| * If that class does not exist, the built-in renderer will be used. | ||
| * | ||
| * @param \Throwable $exception Exception to handle. | ||
| * @param \Throwable|\LogicException|\Cake\Database\Exception|\PHPUnit\Framework\Error\Error $exception Exception to handle. |
There was a problem hiding this comment.
Aren't \LogicException|\Cake\Database\Exception|\PHPUnit\Framework\Error\Error all extending Throwable?
There was a problem hiding this comment.
Yes. Is it more ideal to remove them all and use the inheritable Throwable instead?
There was a problem hiding this comment.
I don't see the point in outlining them all here, if they are subclasses of it.
Also, why removing the typehint?
| } | ||
|
|
||
| // Simulate the global exception handler being invoked. | ||
| if ($this->_exception instanceof Throwable) { |
There was a problem hiding this comment.
Isnt this a behavior change?
They all are instance of this class afaik. Before only in the last catch block this was invoked, now in every case, right?
There was a problem hiding this comment.
Yes, previously it was:
try {
// ...
$this->_response = $response;
} catch (PhpUnitError $e) {
throw $e;
} catch (DatabaseException $e) {
throw $e;
} catch (LogicException $e) {
throw $e;
} catch (Throwable $e) {
$this->_exception = $e;
// Simulate the global exception handler being invoked.
$this->_handleError($e);
}I've updated all throwables to be added to the exception property and then passed to the handleError() to handle the exception. Thoughts?
| } catch (\PHPUnit\Framework\Error\Error $e) { | ||
| $this->_exception = $e; |
There was a problem hiding this comment.
At least PHPUnit exceptions should be rethrown. Normal applications shouldn't use PHPUnit classes.
However it could be widened to \PHPUnit\Framework\Exception class or \PHPUnit\Exception interface.
All PHPUnit asserts should immediately abort test, but not rendered with ExceptionRenderer. Otherwise it makes testing hard.
|
Can you explain again please what the reason for this is? etc It would be good to have the main goal again in front of us, to be able to find a solution. |
|
Closing in favour of #14549 which achieves a similar result with a simpler approach. |
IntegrationTestTrait removed all throwables from send request methods:
See discussion/issue at:
#14143