8000 [Process] Fix ProcessTest - testIgnoringSignal for local by thibaut22200 · Pull Request #57767 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 26, 2024
Merged

[Process] Fix ProcessTest - testIgnoringSignal for local #57767

merged 1 commit into from
Jul 26, 2024

Conversation

thibaut22200
Copy link
Contributor
@thibaut22200 thibaut22200 commented Jul 18, 2024 •
edited
Loading
Q A
Branch? 7.1
Bug fix? no
New feature? no
Deprecations? no
Issues Fix Process test (testIgnoringSignal)
License MIT

Fix ProcessTest : testIgnoringSignal().
Tests on 2 independant env. The test was failed.
Have to pass an array instead of string in the getProcess()

@carsonbot carsonbot added this to the 7.1 milestone Jul 18, 2024
derrabus
derrabus requested changes Jul 18, 2024
View reviewed changes
Copy link
Member
@derrabus derrabus left a 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. The getProcess() method is supposed to handle both, arrays and strings.

@carsonbot carsonbot changed the title Fix ProcessTest - testIgnoringSignal for local [Process] Fix ProcessTest - testIgnoringSignal for local Jul 18, 2024
nicolas-grekas
nicolas-grekas approved these changes Jul 18, 2024
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

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.

derrabus
derrabus approved these changes Jul 18, 2024
View reviewed changes
Copy link
Member
@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right then.

fabpot
fabpot reviewed Jul 19, 2024
View reviewed changes
src/Symfony/Component/Process/Tests/ProcessTest.php
@@ -1669,7 +1669,7 @@ public function testIgnoringSignal()
$this->markTestSkipped('pnctl extension is required.');
}

$process = $this->getProcess('sleep 10');
$process = $this->getProcess(['sleep', '10']);
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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 missing setIgnoredSignals() call IMO to keep them as similar as possible.

@thibaut22200 thibaut22200 force-pushed the 7.1 branch 2 times, most recently from 40b956e to 51ab848 Compare July 24, 2024 13:58
@xabbuh xabbuh mentioned this pull request Jul 24, 2024
Merged
xabbuh
xabbuh requested changes Jul 26, 2024
View reviewed changes
src/Symfony/Component/Process/Tests/ProcessTest.php
@@ -1669,7 +1669,7 @@ public function testIgnoringSignal()
$this->markTestSkipped('pnctl extension is required.');
}

$process = $this->getProcess('sleep 10');
$process = $this->getProcess(['sleep', '10']);
Copy link
Member

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 missing setIgnoredSignals() call IMO to keep them as similar as possible.

@nicolas-grekas
Copy link
Member

Thank you @thibaut22200.

@nicolas-grekas nicolas-grekas merged commit 1dbc465 into symfony:7.1 Jul 26, 2024
7 of 10 checks passed
@thibaut22200 thibaut22200 deleted the 7.1 branch July 26, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot left review comments

@xabbuh xabbuh xabbuh requested changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Process Status: Needs Work
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
6 participants
@thibaut22200 @nicolas-grekas @fabpot @derrabus @xabbuh @carsonbot
0