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 — bpo-46198: rename duplicate tests and remove unused code by sobolevn · Pull Request #30297 · python/cpython

Conversation

@sobolevn

@sobolevn sobolevn commented Dec 30, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Right now, two tests have the same name:

  1. cpython/Lib/test/test_email/test__header_value_parser.py

    Line 304 in 8e11237

    def test_get_unstructured_invalid_ew(self):

  2. cpython/Lib/test/test_email/test__header_value_parser.py

    Line 398 in 8e11237

    def test_get_unstructured_invalid_ew(self):

The first one is always skipped.
I think that NEWS should not be added.

https://bugs.python.org/issue46198

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sobolevn
sobolevn requested a review from a team as a code owner December 30, 2021 00:26
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Dec 30, 2021
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Dec 30, 2021
@AlexWaygood AlexWaygood added the skip news label Dec 30, 2021
@sobolevn

sobolevn commented Dec 30, 2021

Copy link
Copy Markdown
Member Author

test_asyncio is still failing sometimes on Windows 🤔
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sobolevn sobolevn closed this Dec 30, 2021
@sobolevn sobolevn reopened this Dec 30, 2021
@sobolevn sobolevn closed this Dec 30, 2021
@sobolevn sobolevn reopened this Dec 30, 2021
@AlexWaygood AlexWaygood added needs backport to 3.9 labels Dec 30, 2021
AlexWaygood
AlexWaygood approved these changes Dec 30, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Dec 30, 2021
@sobolevn
sobolevn requested review from Fidget-Spinner, abalkin, ethanfurman, gvanrossum and pganssle as code owners January 1, 2022 09:30
@sobolevn

sobolevn commented Jan 1, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I am addressing feedback from https://bugs.python.org/issue46198, so I am increasing the scope of this PR to also include other similar cases. I am using flake8 with --select=F811 to find duplicates.

Amount of tests:

  • test_enum: 232 -> 236
  • test__header_value_parser.py: 1659 -> 1660
  • test_dict: 108 -> 109
  • test_compile: 101 -> 102
  • test_typing: 407 -> 408
All reactions

Sorry, something went wrong.

Uh oh!

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

AlexWaygood commented Jan 1, 2022

Copy link
Copy Markdown
Member

Title of the PR needs to change!
sobolevn 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.
@sobolevn sobolevn changed the title bpo-46198: unskip one test in test_email bpo-46198: rename duplicate tests and remove unused code Jan 1, 2022
merwok
merwok approved these changes Jan 1, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jan 1, 2022
merwok
merwok reviewed Jan 2, 2022
View reviewed changes
Comment thread Lib/test/test_enum.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner
Fidget-Spinner approved these changes Jan 4, 2022
View reviewed changes
merwok
merwok reviewed Jan 4, 2022
View reviewed changes
Comment thread Lib/test/test_enum.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
raghavthind2005
raghavthind2005 approved these changes Jan 18, 2022
View reviewed changes
@sobolevn
Merge branch 'main' into issue-46198
bc5a8a2
@sobolevn

sobolevn commented Jan 26, 2022

Copy link
Copy Markdown
Member Author

Rebased to solve conflicts in test_typing.
All reactions

Sorry, something went wrong.

Uh oh!

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

merwok commented Jan 26, 2022

Copy link
Copy Markdown
Member

In general, please do merges for CPython rather than rebases and force pushes: https://devguide.python.org/pullrequest/
The reviewing experience on github is poor when there are force pushes.
sobolevn 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.
@merwok

merwok commented Jan 30, 2022

Copy link
Copy Markdown
Member

@raghavthind2005 hello 👋🏽 Reviews by non-core developers are useful if you carefully consider the bug report, the changes, the discussions. Sometimes you need to get the branch locally, build python and try out the code to see if it does what it’s supposed to. It is useful if people who are not core devs do these things and write a message on the PR to say that. But just giving +1 without engaging with the process or the people has little value.
AlexWaygood 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.
JelleZijlstra
JelleZijlstra approved these changes Mar 10, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks, I'm planning to merge this (cc @gvanrossum).

The duplicate test cases are quite bad and we should definitely fix them. The duplicate imports aren't as bad but it's still cleaner to get them fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
merwok reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@JelleZijlstra JelleZijlstra self-assigned this Mar 10, 2022
merwok
merwok approved these changes Mar 10, 2022
View reviewed changes
@gvanrossum

gvanrossum commented Mar 10, 2022

Copy link
Copy Markdown
Member

@JelleZijlstra Looks fine to me. (In general I think you don't have to CC me on things you plan to merge any more, unless you really want me to have another look. You're doing great!)
All reactions

Sorry, something went wrong.

Uh oh!

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

JelleZijlstra commented Mar 10, 2022

Copy link
Copy Markdown
Member

Great, thanks! I was thinking of asking when I could leave new core dev quarantine :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JelleZijlstra
JelleZijlstra merged commit 6c83c8e into python:main Mar 10, 2022
@miss-islington

miss-islington commented Mar 10, 2022

Copy link
Copy Markdown
Contributor

Thanks @sobolevn for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 10, 2022
@miss-islington

miss-islington commented Mar 10, 2022

Copy link
Copy Markdown
Contributor

Sorry @sobolevn and @JelleZijlstra, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 6c83c8e6b56b57a8a794e7b6c07837be4ce3bb97 3.10
All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington commented Mar 10, 2022

Copy link
Copy Markdown
Contributor

Sorry, @sobolevn and @JelleZijlstra, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6c83c8e6b56b57a8a794e7b6c07837be4ce3bb97 3.9
All reactions

Sorry, something went wrong.

Uh oh!

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

JelleZijlstra commented Mar 10, 2022

Copy link
Copy Markdown
Member

I'll backport
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this pull request Mar 10, 2022
@sobolevn @JelleZijlstra
[3.10] bpo-46198: rename duplicate tests and remove unused code (pyth…
c031cf4 
…onGH-30297).

(cherry picked from commit 6c83c8e)

Co-authored-by: Nikita Sobolev <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Mar 10, 2022
@bedevere-bot

bedevere-bot commented Mar 10, 2022

Copy link
Copy Markdown

GH-31797 is a backport of this pull request to the 3.9 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this pull request Mar 10, 2022
@sobolevn @JelleZijlstra
[3.9] bpo-46198: rename duplicate tests and remove unused code (pytho…
766f920 
…nGH-30297).

(cherry picked from commit 6c83c8e)

Co-authored-by: Nikita Sobolev <[email protected]>
@JelleZijlstra

JelleZijlstra commented Mar 10, 2022

Copy link
Copy Markdown
Member

#31796 is the 3.10 backport, not sure why the bot didn't pick it up
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood added a commit to AlexWaygood/cpython that referenced this pull request Mar 10, 2022
@AlexWaygood
bpo-46198: Fix test_asyncio.test_sslproto
b9e02a7 
pythonGH-30297 removed a duplicate `from test import support` statement from `test_asyncio.test_sslproto`. However, in between that PR being filed and it being merged, pythonGH-31275 removed the _other_ `from test import support` statement. This means that `support` is now undefined in `test_asyncio.test_sslproto`, causing the CI to fail on all platforms for all PRS.
@AlexWaygood AlexWaygood mentioned this pull request Mar 10, 2022
bpo-46198: Fix test_asyncio.test_sslproto #31801
Merged
JelleZijlstra added a commit that referenced this pull request Mar 10, 2022
@JelleZijlstra @sobolevn
[3.9] bpo-46198: rename duplicate tests and remove unused code (GH-30297
f7f7838 
) (GH-31797)

(cherry picked from commit 6c83c8e)

Co-authored-by: Nikita Sobolev <[email protected]>
JelleZijlstra pushed a commit that referenced this pull request Mar 10, 2022
@AlexWaygood
bpo-46198: Fix test_asyncio.test_sslproto (GH-31801)
4052dd2 
GH-30297 removed a duplicate `from test import support` statement from `test_asyncio.test_sslproto`. However, in between that PR being filed and it being merged, GH-31275 removed the _other_ `from test import support` statement. This means that `support` is now undefined in `test_asyncio.test_sslproto`, causing the CI to fail on all platforms for all PRS.
JelleZijlstra added a commit that referenced this pull request Mar 10, 2022
@JelleZijlstra @sobolevn
[3.10] bpo-46198: rename duplicate tests and remove unused code (GH-3…
4199b7f 
…0297) (GH-31796)

(cherry picked from commit 6c83c8e)

Co-authored-by: Nikita Sobolev <[email protected]>
@hugovk hugovk mentioned this pull request Mar 15, 2022
Accessibility: Add aria-label to clearly describe the dark mode button python/peps#2426
Merged
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
@JelleZijlstra @sobolevn @hello-adam
[3.9] bpo-46198: rename duplicate tests and remove unused code (pytho…
4ac290d 
…nGH-30297) (pythonGH-31797)

(cherry picked from commit 6c83c8e)

Co-authored-by: Nikita Sobolev <[email protected]>
@terryjreedy terryjreedy removed the needs backport to 3.10 label Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok approved these changes
@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes
@AlexWaygood AlexWaygood AlexWaygood approved these changes
@abalkin abalkin Awaiting requested review from abalkin
@ethanfurman ethanfurman Awaiting requested review from ethanfurman ethanfurman is a code owner
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner
+1 more reviewer
@raghavthind2005 raghavthind2005 raghavthind2005 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@JelleZijlstra JelleZijlstra

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.

11 participants

@sobolevn @AlexWaygood @merwok @gvanrossum @JelleZijlstra @miss-islington @bedevere-bot @Fidget-Spinner @raghavthind2005 @the-knights-who-say-ni @terryjreedy
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.