Skip to content

gh-63882: Use self.assert* in test_minidom #133000

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

Merged
merged 6 commits into from
Apr 26, 2025

Conversation

StanFromIreland
Copy link
Contributor

@StanFromIreland StanFromIreland commented Apr 26, 2025

Replaces all single line asserts, all the old confirms that have multiple lines will be in another pr as the diff is harder to view.

Once raised in #63882 (comment):

In fact, having looked at the test module in a bit more detail, it's in pretty sore need of an overall modernization.

And also a few times in #132879

Request: @picnixz , @hugovk , @zware

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

When there are assertTrue(not ...), use assertFalse, and when there are assertTrue(x is y), use assertIs. In addition, confirm() may have more than 1 conditions so they should be broken down.

@bedevere-app
Copy link

bedevere-app bot commented Apr 26, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@StanFromIreland
Copy link
Contributor Author

I messed up and re-ran a broken script after, causing some issues. Sent a fixup

@StanFromIreland StanFromIreland requested a review from picnixz April 26, 2025 11:55
@StanFromIreland
Copy link
Contributor Author

That should be all now, I hope…

@StanFromIreland StanFromIreland requested a review from picnixz April 26, 2025 12:09
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Whenever there is "is(X, None)", use assertIsNone. I won't correct more.

@StanFromIreland StanFromIreland requested a review from picnixz April 26, 2025 17:47
@picnixz picnixz enabled auto-merge (squash) April 26, 2025 18:00
@picnixz picnixz merged commit 56c88e4 into python:main Apr 26, 2025
38 checks passed
@StanFromIreland StanFromIreland deleted the minidom-assert branch April 26, 2025 18:44
@StanFromIreland
Copy link
Contributor Author

Thanks :-)

@hugovk
Copy link
Member

hugovk commented Apr 26, 2025

And backport?

@picnixz
Copy link
Member

picnixz commented Apr 26, 2025

Mmh, yes backports can be done to ease future bps

@hugovk hugovk added the needs backport to 3.13 bugs and security fixes label Apr 26, 2025
@miss-islington-app
Copy link

Thanks @StanFromIreland for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 26, 2025
@bedevere-app
Copy link

bedevere-app bot commented Apr 26, 2025

GH-133024 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Apr 26, 2025
hugovk pushed a commit that referenced this pull request Apr 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants