Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-115596: Fix ProgramPriorityTests in test_os permanently changing the process priority #115610

Merged
merged 6 commits into from Feb 17, 2024

Conversation

brianschubert
Copy link
Contributor

@brianschubert brianschubert commented Feb 17, 2024

Fixes #115596

Runs the priority setting logic from ProgramPriorityTests.test_set_get_priority in a subprocess to prevent the priority change from taking permanent effect.

@bedevere-app bedevere-app bot added the tests Tests in the Lib/test dir label Feb 17, 2024
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

When the test fails, we only see something like:

subprocess.CalledProcessError: Command '['/home/serhiy/py/cpython/python', '-c', 'if 1:\n        import os\n        os.setpriority(os.PRIO_PROCESS, os.getpid(), 0 + 1)\n        print(os.getpriority(os.PRIO_PROCESS, os.getpid()))\n        ']' returned non-zero exit status 1.

Which does not give any information about the cause of the failure. You can use assert_python_ok() which shows the stdout and the stderr of the failed process.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Pure test PRs usually do not need a NEWS entry, but this bug potentially had a large scope.

@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Feb 17, 2024
@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) February 17, 2024 16:06
@serhiy-storchaka serhiy-storchaka merged commit 90dd653 into python:main Feb 17, 2024
36 checks passed
@miss-islington-app
Copy link

Thanks @brianschubert for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 17, 2024
…ging the process priority (pythonGH-115610)

(cherry picked from commit 90dd653)

Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Feb 17, 2024

GH-115616 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Feb 17, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 17, 2024
…ging the process priority (pythonGH-115610)

(cherry picked from commit 90dd653)

Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Feb 17, 2024

GH-115617 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Feb 17, 2024
serhiy-storchaka pushed a commit that referenced this pull request Feb 17, 2024
…nging the process priority (GH-115610) (GH-115616)

(cherry picked from commit 90dd653)

Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
serhiy-storchaka pushed a commit that referenced this pull request Feb 17, 2024
…nging the process priority (GH-115610) (GH-115617)

(cherry picked from commit 90dd653)

Co-authored-by: Brian Schubert <brianm.schubert@gmail.com>
woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_os permanently change the process priority
2 participants