Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-46198: rename duplicate tests and remove unused code#30297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
sobolevn commented Dec 30, 2021 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
sobolevn commented Dec 30, 2021
|
sobolevn commented Jan 1, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 Amount of tests:
|
AlexWaygood commented Jan 1, 2022
Title of the PR needs to change! |
test_emailUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
sobolevn commented Jan 26, 2022
Rebased to solve conflicts in |
merwok commented Jan 26, 2022
In general, please do merges for CPython rather than rebases and force pushes: https://devguide.python.org/pullrequest/ |
merwok commented Jan 30, 2022
@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 left a comment
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.
gvanrossum commented Mar 10, 2022
@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 commented Mar 10, 2022
Great, thanks! I was thinking of asking when I could leave new core dev quarantine :) |
miss-islington commented Mar 10, 2022
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 commented Mar 10, 2022
Sorry @sobolevn and @JelleZijlstra, I had trouble checking out the |
miss-islington commented Mar 10, 2022
Sorry, @sobolevn and @JelleZijlstra, I could not cleanly backport this to |
JelleZijlstra commented Mar 10, 2022
I'll backport |
…onGH-30297). (cherry picked from commit 6c83c8e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
bedevere-bot commented Mar 10, 2022
GH-31797 is a backport of this pull request to the 3.9 branch. |
…nGH-30297). (cherry picked from commit 6c83c8e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
JelleZijlstra commented Mar 10, 2022
#31796 is the 3.10 backport, not sure why the bot didn't pick it up |
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.
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.
…nGH-30297) (pythonGH-31797) (cherry picked from commit 6c83c8e) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
Right now, two tests have the same name:
cpython/Lib/test/test_email/test__header_value_parser.py
Line 304 in 8e11237
cpython/Lib/test/test_email/test__header_value_parser.py
Line 398 in 8e11237
The first one is always skipped.
I think that NEWS should not be added.
https://bugs.python.org/issue46198