Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Process] Fix ProcessTest - testIgnoringSignal for local#57767
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
We should not need this change. ThegetProcess() method is supposed to handle both, arrays and strings.
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
This is not a CS change: this removes the sh wrapper. For some reason, this test case also fails on my laptop and this patch fixes it. Dunno how masking works when there's a shell wrapper but at least when there's none, this proves masking works.
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.
All right then.
| @@ -1669,7 +1669,7 @@ public function testIgnoringSignal() | |||
| $this->markTestSkipped('pnctl extension is required.'); | |||
| } | |||
| $process = $this->getProcess('sleep10'); | |||
| $process = $this->getProcess(['sleep', '10']); | |||
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.
Do we need to change line 1688 as well?
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.
The test works without changing it so... no I guess
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.
While this might be true the comment on the test states that it acts as a prove that the previous test tests what it claims to do. Thus the only difference should be the missingsetIgnoredSignals() call IMO to keep them as similar as possible.
40b956e to51ab848Compare| @@ -1669,7 +1669,7 @@ public function testIgnoringSignal() | |||
| $this->markTestSkipped('pnctl extension is required.'); | |||
| } | |||
| $process = $this->getProcess('sleep10'); | |||
| $process = $this->getProcess(['sleep', '10']); | |||
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.
While this might be true the comment on the test states that it acts as a prove that the previous test tests what it claims to do. Thus the only difference should be the missingsetIgnoredSignals() call IMO to keep them as similar as possible.
Thank you@thibaut22200. |
1dbc465 intosymfony:7.1Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fix ProcessTest : testIgnoringSignal().
Tests on 2 independant env. The test was failed.
Have to pass an array instead of string in the getProcess()