[Process] Fixed fatal errors in getOutput and getErrorOutput when process was not started #10480
Conversation
…cess was not started
|
shouldn't we add tests covering this ? |
| * | ||
| * @throws LogicException If the process has not run. | ||
| */ | ||
| private function requireProcessHasRun($functionName) |
There was a problem hiding this comment.
i would name it requireProcessIsStarted just as $this->isStarted()
|
@stof what tests are missing ? It seems everything is unit tested in this PR, isn't it ? |
|
@Tobion comment addressed |
|
well, the case which are currently throwing a fatal error are not tested. |
| } catch (\Exception $e) { | ||
| $this->assertInstanceOf('Symfony\Component\Process\Exception\LogicException', $e); | ||
| $this->assertEquals(sprintf('Process must be terminated before calling %s.', $method), $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
This test will not fail if the exception is not triggered (try returning null instead of triggering the exception and you will see it pass)
There was a problem hiding this comment.
Aside what @stof said, why not catching the LogicException as opposed to asserting what's the type? The current catch block will also catch any PhpUnit exceptions (like those thrown on warnings). There should be $this->fail() call after call_user_func().
There was a problem hiding this comment.
It's about having a proper Process::stop call in case the exception is not the expected one (last line of the test)
|
PR updated, comments addressed |
| * | ||
| * @see http://tldp.org/LDP/abs/html/exitcodes.html | ||
| * @see http://en.wikipedia.org/wiki/Unix_signal | ||
| */ | ||
| public function getExitCodeText() | ||
| { | ||
| if (null === $this->exitcode) { |
There was a problem hiding this comment.
i think this is problematic since it does not call updateStatus before.
IMO it's better to remove this and add below
$exitcode = $this->getExitCode();
if (null === $exitcode) {
return null;
}
There was a problem hiding this comment.
good catch, it's now fixed
…called after termination
|
+1 |
…ut when process was not started (romainneutron) This PR was merged into the 2.3 branch. Discussion ---------- [Process] Fixed fatal errors in getOutput and getErrorOutput when process was not started | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #10022 | License | MIT This PR replaces #9452 and address the latest changes. Side note : I've not updated `getExitCode`, `getExitCodeText` and `isSuccessful` as they were explicitly tested to return null in case the process is not started or terminated. I think it would be a BC break to do that. Commits ------- 449fe01 [Process] Trow exceptions in case a Process method is supposed to be called after termination 0ae6858 [Process] fixed fatal errors in getOutput and getErrorOutput when process was not started
| public function testExitCodeTextIsNullWhenExitCodeIsNull() | ||
| { | ||
| $process = $this->getProcess(''); | ||
| $this->assertNull($process->getExitCodeText()); |
There was a problem hiding this comment.
@romainneutron I get a failure on Windows 7, PHP 5.5.3, Mingw:
Symfony\Component\Process\Exception\RuntimeException: This PHP has been compiled with --enable-sigchild. You must use setEnhanceSigchildCompatibility() to use this method.
There was a problem hiding this comment.
Yep, I got it : #10483
|
I wonder why Process::restart does Restart to me means stop() followed by start(). Why must the process be finished before restarting? Does not seem logical to me. Compare: If you have a countdown you want to restart, I would not expect that I need to wait until it's done before I may restart the countdown. |
…onment (romainneutron) This PR was merged into the 2.3 branch. Discussion ---------- [2.3][Process] Fix unit tests in sigchild disabled environment | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT We've been a bit fast when merging #10480 and tests were broken in the last update I did. Commits ------- 5f6ee12 [Process] Fix unit tests in sigchild disabled environment
|
Also I wonder why |
|
Also Process::start() |
|
@Tobion your comment makes sense about the exception thrown in About |
|
I just wonder that |
…tests in sigchild environment (romainneutron) This PR was merged into the 2.3 branch. Discussion ---------- [2.3][Process] Remove unreachable code + avoid skipping tests in sigchild environment | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT As mentioned by @Tobion in #10480 (comment), I removed the dead code. I also fixed/updated the test suite on PHP compiled with `--enable-sigchild`. Commits ------- d52dd32 [Process] Remove unreachable code + avoid skipping tests in sigchild environment
…tests in sigchild environment (romainneutron) This PR was merged into the 2.3 branch. Discussion ---------- [2.3][Process] Remove unreachable code + avoid skipping tests in sigchild environment | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT As mentioned by @Tobion in symfony/symfony#10480 (comment), I removed the dead code. I also fixed/updated the test suite on PHP compiled with `--enable-sigchild`. Commits ------- d52dd32 [Process] Remove unreachable code + avoid skipping tests in sigchild environment
This PR replaces #9452 and address the latest changes.
Side note : I've not updated
getExitCode,getExitCodeTextandisSuccessfulas they were explicitly tested to return null in case the process is not started or terminated. I think it would be a BC break to do that.