◐ Shell
reader mode source ↗
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

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

    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

@sobolevn sobolevn requested a review from a team as a code owner December 30, 2021 00:26
@sobolevn

sobolevn commented Dec 30, 2021

Copy link
Copy Markdown
Member Author

test_asyncio is still failing sometimes on Windows 🤔

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

sobolevn commented Jan 1, 2022 •
edited
Loading

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

@AlexWaygood

AlexWaygood commented Jan 1, 2022

Copy link
Copy Markdown
Member

Title of the PR needs to change!

@sobolevn sobolevn changed the title bpo-46198: unskip one test in test_email Jan 1, 2022
16 hidden items Load more…
@sobolevn

sobolevn commented Jan 26, 2022

Copy link
Copy Markdown
Member Author

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

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

Hide comment

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.

@JelleZijlstra JelleZijlstra 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: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.
🐍🍒⛏🤖

@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.
@AlexWaygood AlexWaygood 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: 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 <mail@sobolevn.me>
@hugovk hugovk 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>
@terryjreedy terryjreedy removed the needs backport to 3.10 only security fixes 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

@JelleZijlstra JelleZijlstra

@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

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.