Skip to content

Specific asserts #205

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 8 commits into from
Apr 19, 2016
Merged

Specific asserts #205

merged 8 commits into from
Apr 19, 2016

Conversation

zware
Copy link
Member

@zware zware commented Apr 18, 2016

This PR converts the tests from simple assert statements to TestCase.assert* calls. This ensures that the tests actually test something even if Python is run with -O, and makes the tests fit the style of the rest of the standard library tests.

Most of the changes were made by simple s/assert .../self.assert.../ replacements, with a few manual touchups.

zware added 7 commits April 18, 2016 16:00
Also makes use of 'self.assertIsSubclass' and 'self.assertNotIsSubclass'
rather than 'assert issubclass' and 'assert not issubclass'
s/assert isinstance/self.assertIsInstance/
s/assert not isinstance/self.assertNotIsInstance/
@gvanrossum
Copy link
Member

I would actually prefer going the other way -- change the remaining self.assertEqual(x, y) calls to assert x == y. I mostly run the tests using py.test and it much prefers the latter idiom.

Does anyone routinely run the stdlib tests with -O?

@zware
Copy link
Member Author

zware commented Apr 18, 2016 via email

@@ -20,6 +20,25 @@
import typing


class TestCase(_TestCase):

# vanilla TestCase doesn't have assert*Subclass, see bugs.python.org/14819
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it's not going to happen any time soon. I propose not to count on it. The rename dance TestCase -> _TestCase feels kind of awkward -- why not just pick a decent new name for TestCase + assert[Not]IsSubclass, and use that where it's needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, the comment was not meant to imply that assertIsSubclass would eventually be a standard feature. The rename dance was an effort to avoid churn, but as you pointed out, the whole PR is pretty much churn so I'll just change it to BaseTestCase.

@gvanrossum
Copy link
Member

OK, I concede your point (even though I don't like noisy commits -- you will end up owning all those lines in hg/git blame forever). Once this is merged here, can you take care of merging it into 3.5+3.6?

@zware
Copy link
Member Author

zware commented Apr 19, 2016

My ownership of the lines is unfortunate, agreed.

Yes, I can sync cpython after merge here.

@bintoro
Copy link
Contributor

bintoro commented Apr 19, 2016

Note that, once #136 is resolved, most issubclass assertions will have to be changed again.

@gvanrossum gvanrossum merged commit e599f84 into python:master Apr 19, 2016
@gvanrossum
Copy link
Member

OK, go ahead and merge into CPython 3.5 and 3.6.

@zware
Copy link
Member Author

zware commented Apr 19, 2016

Thanks, Guido. I've merged into cpython 3.5 and default.

The only remaining question is whether you want python2/test_typing.py similarly treated. I have no opinion about it, but am willing to put forth the effort to do it if you want to keep the diff between src and python2 smaller.

@zware zware deleted the specific_asserts branch April 19, 2016 16:56
@gvanrossum
Copy link
Member

Yea, please do the python2/test_typing.py -- I do try to keep them in sync. And there are plenty of PRs coming up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants