Skip to content

bpo-46198: rename duplicate tests and remove unused code - #30297

Merged
JelleZijlstra merged 3 commits into
python:mainfrom
sobolevn:issue-46198
Mar 10, 2022
Merged

bpo-46198: rename duplicate tests and remove unused code#30297
JelleZijlstra merged 3 commits into
python:mainfrom
sobolevn:issue-46198

Conversation

@sobolevn

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

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

    deftest_get_unstructured_invalid_ew(self):

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

    Line 398 in 8e11237

    deftest_get_unstructured_invalid_ew(self):

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

https://bugs.python.org/issue46198

@sobolevn
sobolevn requested a review from a team as a code ownerDecember 30, 2021 00:26
@bedevere-botbedevere-bot added tests Tests in the Lib/test dir awaiting review labels Dec 30, 2021
@sobolevn

sobolevn commented Dec 30, 2021

Copy link
Copy Markdown
MemberAuthor

test_asyncio is still failing sometimes on Windows 🤔

@sobolevnsobolevn reopened this Dec 30, 2021
@sobolevnsobolevn reopened this Dec 30, 2021
@sobolevn

sobolevn commented Jan 1, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

@AlexWaygood

AlexWaygood commented Jan 1, 2022

Copy link
Copy Markdown
Member

Title of the PR needs to change!

@sobolevnsobolevn changed the title bpo-46198: unskip one test in test_emailbpo-46198: rename duplicate tests and remove unused codeJan 1, 2022
merwok
merwok reviewed Jan 2, 2022
View reviewed changes
Comment threadLib/test/test_enum.py Outdated
merwok
merwok reviewed Jan 4, 2022
View reviewed changes
Comment threadLib/test/test_enum.py Outdated
@sobolevn

sobolevn commented Jan 26, 2022

Copy link
Copy Markdown
MemberAuthor

Rebased to solve conflicts in test_typing.

@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.

@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.

JelleZijlstra
JelleZijlstra approved these changes Mar 10, 2022
View reviewed changes

@JelleZijlstraJelleZijlstra left a comment

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.

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.

@JelleZijlstraJelleZijlstra self-assigned this Mar 10, 2022
@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!)

@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 :)

@JelleZijlstra
JelleZijlstra merged commit 6c83c8e into python:mainMar 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.
🐍🍒⛏🤖

@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

@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

@JelleZijlstra

JelleZijlstra commented Mar 10, 2022

Copy link
Copy Markdown
Member

I'll backport

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 <mail@sobolevn.me>
@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.

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 <mail@sobolevn.me>
@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

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.
@AlexWaygoodAlexWaygood mentioned this pull request Mar 10, 2022
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 <mail@sobolevn.me>
JelleZijlstra pushed a commit that referenced this pull request Mar 10, 2022
@AlexWaygood
bpo-46198: Fixtest_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 <mail@sobolevn.me>
@hugovkhugovk mentioned this pull request Mar 15, 2022
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 <mail@sobolevn.me>
@terryjreedyterryjreedy removed the needs backport to 3.10 only security fixes label Feb 18, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwokmerwokmerwok approved these changes
@JelleZijlstraJelleZijlstraJelleZijlstra approved these changes
@Fidget-SpinnerFidget-SpinnerFidget-Spinner approved these changes
@AlexWaygoodAlexWaygoodAlexWaygood approved these changes
@abalkinabalkinAwaiting requested review from abalkin
@ethanfurmanethanfurmanAwaiting requested review from ethanfurmanethanfurman is a code owner
@gvanrossumgvanrossumAwaiting requested review from gvanrossum
@pgansslepganssleAwaiting requested review from pgansslepganssle is a code owner
+1 more reviewer
@raghavthind2005raghavthind2005raghavthind2005 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@JelleZijlstraJelleZijlstra

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@sobolevn@AlexWaygood@merwok@gvanrossum@JelleZijlstra@miss-islington@bedevere-bot@Fidget-Spinner@raghavthind2005@the-knights-who-say-ni@terryjreedy