GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

Conversation

@aisk

@aisk aisk commented Jan 9, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Related to #102731 (comment).

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aisk
improve the assert for test_one_environment_variable
4f22675
@aisk
aisk requested a review from gpshead as a code owner January 9, 2024 16:42
@bedevere-app bedevere-app Bot added tests Tests in the Lib/test dir awaiting review labels Jan 9, 2024
@aisk aisk mentioned this pull request Jan 9, 2024
gh-87868: Sort and remove duplicates in getenvironment() #102731
Merged
@aisk aisk changed the title gh-102731: improve the assert for test_one_environment_variable gh-87868: improve the assert for test_one_environment_variable Jan 9, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jan 9, 2024
[_winapi] correctly sort and remove duplicates in getenvironment() #87868
Closed
@erlend-aasland erlend-aasland added the skip news label Jan 9, 2024
@erlend-aasland

erlend-aasland commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aisk 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_variable Jan 9, 2024
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 9, 2024
@bedevere-bot

bedevere-bot commented Jan 9, 2024

Copy link
Copy Markdown

🤖 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 9, 2024
@aisk

aisk commented Jan 9, 2024

Copy link
Copy Markdown
Member Author

How about this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aisk

aisk commented Jan 9, 2024

Copy link
Copy Markdown
Member Author

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 and zooba reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zooba

zooba commented Jan 9, 2024

Copy link
Copy Markdown
Member

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)
erlend-aasland and aisk reacted with thumbs up emoji erlend-aasland reacted with laugh emoji
All reactions
  • 👍 2 reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aisk

aisk commented Jan 9, 2024

Copy link
Copy Markdown
Member Author

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.
erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zooba
zooba reviewed Jan 9, 2024
View reviewed changes
Comment thread Lib/test/test_subprocess.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aisk @zooba
Update Lib/test/test_subprocess.py
a982151 
Co-authored-by: Steve Dower <[email protected]>
@aisk 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_subprocess Jan 9, 2024
@erlend-aasland

This comment was marked as off-topic.

Sign in to view
@aisk

aisk commented Jan 9, 2024

Copy link
Copy Markdown
Member Author

The failed test environment is Linux based, like https://buildbot.python.org/all/#/builders/543/builds/5159 .
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

This comment was marked as outdated.

Sign in to view
@bedevere-bot

This comment was marked as off-topic.

Sign in to view
@erlend-aasland

erlend-aasland commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

This comment was marked as outdated.

Sign in to view
@bedevere-bot

This comment was marked as outdated.

Sign in to view
zooba
zooba reviewed Jan 9, 2024
View reviewed changes
Comment thread Lib/test/test_subprocess.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aisk @zooba
Update Lib/test/test_subprocess.py
6788414 
Co-authored-by: Steve Dower <[email protected]>
@aisk 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-shared Jan 10, 2024
@erlend-aasland

This comment was marked as outdated.

Sign in to view
@bedevere-bot

This comment was marked as outdated.

Sign in to view
@aisk
also skip the test if AddressSanitizer is enabled
5671246
@aisk

aisk commented Jan 10, 2024

Copy link
Copy Markdown
Member Author

Sorry,I updated it again, the AddressSanitizer` should be skipped also.
erlend-aasland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Jan 10, 2024

Copy link
Copy Markdown
Contributor

!buildbot .64 (RHEL|Fedora Stable).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Jan 10, 2024

Copy link
Copy Markdown

🤖 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zooba

zooba commented Jan 10, 2024

Copy link
Copy Markdown
Member

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aisk

aisk commented Jan 10, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.
zooba, gpshead, and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead approved these changes Jan 10, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jan 10, 2024
@gpshead gpshead added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jan 10, 2024
@gpshead
gpshead merged commit fafb327 into python:main Jan 10, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 10, 2024
@miss-islington-app

miss-islington-app Bot commented Jan 10, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

miss-islington-app Bot commented Jan 10, 2024

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app miss-islington-app Bot assigned gpshead Jan 10, 2024
@gpshead gpshead removed needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jan 10, 2024
@miss-islington-app

miss-islington-app Bot commented Jan 10, 2024

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead

gpshead commented Jan 10, 2024

Copy link
Copy Markdown
Member

oh, backports do not appear to be needed. yay.
erlend-aasland and aisk reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead

gpshead commented Jan 10, 2024

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead pushed a commit to miss-islington/cpython that referenced this pull request Jan 10, 2024
@aisk @zooba @gpshead
pythongh-87868: Skip test_one_environment_variable in `test_subproc…
84a5a59 
…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 <[email protected]>
gpshead pushed a commit to miss-islington/cpython that referenced this pull request Jan 10, 2024
@aisk @zooba @gpshead
pythongh-87868: Skip test_one_environment_variable in `test_subproc…
03022e4 
…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 <[email protected]>
@aisk
aisk deleted the improve_test_one_environment_variable branch January 16, 2024 10:14
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
@aisk @zooba @kulikjak
pythongh-87868: Skip test_one_environment_variable in `test_subproc…
aa2027a 
…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 <[email protected]>
aisk added a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@aisk @zooba
pythongh-87868: Skip test_one_environment_variable in `test_subproc…
9b8a723 
…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 <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@aisk @zooba @Glyphack
pythongh-87868: Skip test_one_environment_variable in `test_subproc…
17dfdb8 
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zooba zooba zooba left review comments
@gpshead gpshead gpshead approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@aisk @erlend-aasland @bedevere-bot @zooba @gpshead
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.