Skip to content

bpo-35537: Rewrite setsid test for os.posix_spawn - #11721

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:posix_spawn_setsid_test
Jun 14, 2019
Merged

bpo-35537: Rewrite setsid test for os.posix_spawn#11721
vstinner merged 1 commit into
python:masterfrom
vstinner:posix_spawn_setsid_test

Conversation

@vstinner

@vstinnervstinner commented Feb 1, 2019•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

bpo-35537, bpo-35876: Fix also test_start_new_session() of
test_subprocess: use os.getsid() rather than os.getpgid().

https://bugs.python.org/issue35537

@bedevere-botbedevere-bot added tests Tests in the Lib/test dir awaiting merge labels Feb 1, 2019
@vstinner

vstinner commented Feb 1, 2019

Copy link
Copy Markdown
MemberAuthor

@nanjekyejoannah, @izbyshev: Would you mind to review my PR?

Alexey: I also fixed test_subprocess.

izbyshev
izbyshev reviewed Feb 1, 2019
View reviewed changes

@izbyshevizbyshev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM except several small issues.

Comment threadLib/test/test_posix.py Outdated
Comment threadLib/test/test_posix.py Outdated
Comment threadLib/test/test_posix.py Outdated
@vstinner
bpo-35537: Rewrite setsid test for os.posix_spawn
6dbc3a1
bpo-35537, bpo-35876: Fix also test_start_new_session() of
test_subprocess: use os.getsid() rather than os.getpgid().
@vstinner

vstinner commented Jun 14, 2019

Copy link
Copy Markdown
MemberAuthor

@izbyshev: Would you mind to review my update PR?

gpshead
gpshead approved these changes Jun 14, 2019
View reviewed changes
Comment threadLib/test/test_subprocess.py
if e.errno != errno.EPERM:
raise
else:
parent_pgid = os.getpgid(os.getpid())

@gpsheadgpsheadJun 14, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i'm wondering if there is a reason this code was calling os.getpgid() rather than os.getsid() but I honestly do not remember. We added this test 9 years ago with the _posixsubprocess.c internals rewrite, regardless of that calling setsid(). My PGID vs SID knowledge is vague and needs refreshing, they're probably the same in this situation?

@vstinnervstinnerJun 14, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I cannot tell you why you wrote this code :-)

@vstinner
vstinner merged commit 5884043 into python:masterJun 14, 2019
@vstinner
vstinner deleted the posix_spawn_setsid_test branch June 14, 2019 17:31
@miss-islington

miss-islington commented Jun 14, 2019

Copy link
Copy Markdown
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot

bedevere-bot commented Jun 14, 2019

Copy link
Copy Markdown

GH-14093 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Jun 14, 2019
@miss-islington@vstinner
bpo-35537: Rewrite setsid test for os.posix_spawn (GH-11721)
e696b15
bpo-35537, bpo-35876: Fix also test_start_new_session() of
test_subprocess: use os.getsid() rather than os.getpgid().
(cherry picked from commit 5884043)
Co-authored-by: Victor Stinner <vstinner@redhat.com>
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@vstinner@lisroach
bpo-35537: Rewrite setsid test for os.posix_spawn (pythonGH-11721)
2f43619
bpo-35537, bpo-35876: Fix also test_start_new_session() of
test_subprocess: use os.getsid() rather than os.getpgid().
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@vstinner@DinoV
bpo-35537: Rewrite setsid test for os.posix_spawn (pythonGH-11721)
04c2b60
bpo-35537, bpo-35876: Fix also test_start_new_session() of
test_subprocess: use os.getsid() rather than os.getpgid().
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpsheadgpsheadgpshead approved these changes
+1 more reviewer
@izbyshevizbyshevizbyshev left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vstinner@miss-islington@bedevere-bot@gpshead@izbyshev@the-knights-who-say-ni