Infection must return appropriate exit status when failing #1299
Conversation
|
Sure! |
one job has failed with such error, restarted (I'm not sure why only one ;) ) |
|
Thank you, @sanmai |
| @@ -213,7 +213,7 @@ protected function configure(): void | |||
| ; | |||
| } | |||
|
|
|||
| protected function executeCommand(IO $io): void | |||
| protected function executeCommand(IO $io): bool | |||
There was a problem hiding this comment.
how about keeping void here? In practice we tend to throw an exception in case of an error instead of a boolean value
There was a problem hiding this comment.
Do you propose that we re-throw the exceptions instead?
There was a problem hiding this comment.
We actually already do since we don't wrap the whole call in with a catch (Throwable $t) (and Symfony already does it). So I don't think forcing this return code is providing anything tbh
There was a problem hiding this comment.
Well, we do wrap the other call for two very important exceptions.
|
But to log the error and carry on right?
…On Wed 19 Aug 2020 at 15:58, Alexey Kopytko ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/Command/RunCommand.php
<#1299 (comment)>:
> @@ -213,7 +213,7 @@ protected function configure(): void
;
}
- protected function executeCommand(IO $io): void
+ protected function executeCommand(IO $io): bool
Well, we do wrap the other call for two very important exceptions.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1299 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHPVAKVNECU4LJZN5MV2JLSBPLBJANCNFSM4QEW5ULQ>
.
|
|
Well, not during CI. One expects that Infection will break the CI build if MSI is lower than required. |
|
Fair enough |
This PR fixes the unfortunate issue where Infection wouldn't exit with an error status in case of an error (e.g. low MSI).
Tests pending.