gh-87868: Skip some test in `test_subprocess` if python is configured with --enable-shared by aisk · Pull Request #113867 · python/cpython

aisk · 2024-01-09T16:42:21Z

Related to #102731 (comment).

Issue: [_winapi] correctly sort and remove duplicates in getenvironment() #87868

erlend-aasland · 2024-01-09T16:55:07Z

Can you update the PR title to more accurately reflect the change?

bedevere-bot · 2024-01-09T16:58:30Z

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 4f22675 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

aisk · 2024-01-09T16:58:39Z

How about this?

aisk · 2024-01-09T17:03:42Z

I was thinking that perhaps we could determine the PR title later, if we addressed the issue, and fixed it in this PR.

erlend-aasland · 2024-01-09T17:14:33Z

How about this?

Instead of describing the git diff, I would try to write the PR title so it reflects the change in behaviour. For example, a PR can fix a flakey test by making the test environment deterministic. If you are scrolling through the git log it is more helpful to read about what effect a commit had. We can always check the diff by doing git log -p :)

zooba · 2024-01-09T17:38:43Z

I wouldn't expect this to fix it, btw. It's just going to improve the output when it fails. So we can review the buildbot results with this change and figure out the actual fix, and then do that as part of this PR (which will then need a different title)

aisk · 2024-01-09T17:49:07Z

Instead of describing the git diff, I would try to write the PR title so it reflects the change in behaviour. For example, a PR can fix a flakey test by making the test environment deterministic. If you are scrolling through the git log it is more helpful to read about what effect a commit had. We can always check the diff by doing git log -p :)

Really thanks! I've learnt a lot from this.

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

aisk · 2024-01-09T18:30:38Z

The failed test environment is Linux based, like https://buildbot.python.org/all/#/builders/543/builds/5159 .

erlend-aasland · 2024-01-09T21:24:09Z

The failed test environment is Linux based, like https://buildbot.python.org/all/#/builders/543/builds/5159 .

You are right; I assumed it was Windows-specific, since the previous PR was Windows oriented.

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

aisk · 2024-01-10T13:08:47Z

Sorry,I updated it again, the AddressSanitizer` should be skipped also.

erlend-aasland · 2024-01-10T13:30:52Z

!buildbot .64 (RHEL|Fedora Stable).

bedevere-bot · 2024-01-10T13:30:56Z

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 5671246 🤖

The command will test the builders whose names match following regular expression: .*64 (RHEL|Fedora Stable).*

The builders matched are:

AMD64 RHEL8 PR
aarch64 RHEL8 LTO + PGO PR
aarch64 Fedora Stable LTO PR
AMD64 RHEL7 LTO PR
AMD64 RHEL8 Refleaks PR
AMD64 RHEL8 LTO PR
AMD64 Fedora Stable Refleaks PR
AMD64 RHEL8 FIPS No Builtin Hashes PR
aarch64 RHEL8 PR
AMD64 Fedora Stable Clang PR
aarch64 Fedora Stable Refleaks PR
AMD64 Fedora Stable LTO + PGO PR
aarch64 RHEL8 Refleaks PR
AMD64 RHEL7 PR
aarch64 RHEL8 LTO PR
AMD64 Fedora Stable LTO PR
AMD64 RHEL7 LTO + PGO PR
aarch64 Fedora Stable Clang Installed PR
aarch64 Fedora Stable PR
AMD64 Fedora Stable PR
aarch64 Fedora Stable LTO + PGO PR
AMD64 RHEL7 Refleaks PR
aarch64 Fedora Stable Clang PR
AMD64 RHEL8 FIPS Only Blake2 Builtin Hash PR
AMD64 RHEL8 LTO + PGO PR
AMD64 Fedora Stable Clang Installed PR

zooba · 2024-01-10T16:52:14Z

We should also be able to run the test by copying os.environ into the env dict and then adding our own keys, right? That's what I always end up doing to make sure things launch on Windows (would be nice if it was built in... but too late there)

aisk · 2024-01-10T17:06:50Z

The main intent of this test is to determine if we correctly handle the off-by-one problems in the for loops within the sort and de-duplication functions. For example, do we missed something if there is just one element in the environment keys. Therefore I think we can't copy the os.environ in the test.

miss-islington-app · 2024-01-10T23:17:09Z

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

miss-islington-app · 2024-01-10T23:17:23Z

Sorry, @aisk and @gpshead, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker fafb3275f25e116e51ff0b867aec597cb3de840f 3.12

miss-islington-app · 2024-01-10T23:17:39Z

Sorry, @aisk and @gpshead, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker fafb3275f25e116e51ff0b867aec597cb3de840f 3.11

gpshead · 2024-01-10T23:18:13Z

oh, backports do not appear to be needed. yay.

gpshead · 2024-01-10T23:18:46Z

thanks for the fix! this test is indeed somewhat fragile and only easy to run on some platforms - skipping it when it cannot be run is fine. we've still got coverage elsewhere.

…ess` when the platform or build cannot do that (python#113867) * improve the assert for test_one_environment_variable * skip some test in test_subprocess when python is configured with shared * also skip the test if AddressSanitizer is enabled --------- Co-authored-by: Steve Dower <steve.dower@microsoft.com>

improve the assert for test_one_environment_variable
4f22675

aisk requested a review from gpshead as a code owner January 9, 2024 16:42

bedevere-appBot added tests Tests in the Lib/test dir awaiting review labels Jan 9, 2024

aisk mentioned this pull request Jan 9, 2024
gh-87868: Sort and remove duplicates in getenvironment() #102731
Merged

aisk changed the title ~~gh-102731: improve the assert for test_one_environment_variable~~gh-87868: improve the assert for test_one_environment_variableJan 9, 2024

bedevere-appBot mentioned this pull request Jan 9, 2024
[_winapi] correctly sort and remove duplicates in getenvironment() #87868
Closed

erlend-aasland added the skip news label Jan 9, 2024

aisk changed the title ~~gh-87868: improve the assert for test_one_environment_variable~~gh-87868: use assertEqual rather than assertTrue in test_subprocess.test_one_environment_variableJan 9, 2024

erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 9, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 9, 2024

zooba reviewed Jan 9, 2024
View reviewed changes

Comment threadLib/test/test_subprocess.py Outdated

Update Lib/test/test_subprocess.py
a982151
Co-authored-by: Steve Dower <steve.dower@microsoft.com>

aisk changed the title ~~gh-87868: use assertEqual rather than assertTrue in test_subprocess.test_one_environment_variable~~gh-87868: Improve the test output for test_subprocessJan 9, 2024

This comment was marked as off-topic.
Sign in to view

This comment was marked as outdated.
Sign in to view

This comment was marked as off-topic.
Sign in to view

This comment was marked as outdated.
Sign in to view

zooba reviewed Jan 9, 2024
View reviewed changes

Comment threadLib/test/test_subprocess.py Outdated

Update Lib/test/test_subprocess.py
6788414
Co-authored-by: Steve Dower <steve.dower@microsoft.com>

aisk changed the title ~~gh-87868: Improve the test output for test_subprocess~~gh-87868: Skip some test in test_subprocess if python is configured with --enable-sharedJan 10, 2024

This comment was marked as outdated.
Sign in to view

also skip the test if AddressSanitizer is enabled
5671246

gpshead approved these changes Jan 10, 2024
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting review labels Jan 10, 2024

gpshead added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jan 10, 2024

gpshead merged commit fafb327 into python:mainJan 10, 2024

bedevere-appBot removed the awaiting merge label Jan 10, 2024

miss-islington-appBot assigned gpshead Jan 10, 2024

gpshead removed needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jan 10, 2024

aisk deleted the improve_test_one_environment_variable branch January 16, 2024 10:14

Uh oh!

Uh oh!

Conversation

aisk commented Jan 9, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Jan 9, 2024

Uh oh!

bedevere-bot commented Jan 9, 2024

Uh oh!

aisk commented Jan 9, 2024

Uh oh!

aisk commented Jan 9, 2024

Uh oh!

erlend-aasland commented Jan 9, 2024

Uh oh!

zooba commented Jan 9, 2024

Uh oh!

aisk commented Jan 9, 2024

Uh oh!

Uh oh!

This comment was marked as off-topic.

aisk commented Jan 9, 2024

Uh oh!

This comment was marked as outdated.

This comment was marked as off-topic.

erlend-aasland commented Jan 9, 2024

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

aisk commented Jan 10, 2024

Uh oh!

erlend-aasland commented Jan 10, 2024

Uh oh!

bedevere-bot commented Jan 10, 2024

Uh oh!

zooba commented Jan 10, 2024

Uh oh!

aisk commented Jan 10, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miss-islington-appBot commented Jan 10, 2024

Uh oh!

miss-islington-appBot commented Jan 10, 2024

Uh oh!

miss-islington-appBot commented Jan 10, 2024

Uh oh!

gpshead commented Jan 10, 2024

Uh oh!

gpshead commented Jan 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aisk commented Jan 9, 2024•
edited
Loading

aisk commented Jan 10, 2024•
edited
Loading