Skip to content

TST: Fix assertNotIsInstance msg #13091

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

TST: Fix assertNotIsInstance msg #13091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions pandas/tests/test_testing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,21 @@ def test_frame_equal_message(self):
by_blocks=True)


class TestIsInstance(tm.TestCase):

def test_isinstance(self):

expected = "Expected type "
with assertRaisesRegexp(AssertionError, expected):
tm.assertIsInstance(1, pd.Series)

def test_notisinstance(self):

expected = "Input must not be type "
with assertRaisesRegexp(AssertionError, expected):
tm.assertNotIsInstance(pd.Series([1]), pd.Series)


class TestRNGContext(unittest.TestCase):

def test_RNGContext(self):
Expand Down
12 changes: 6 additions & 6 deletions pandas/util/testing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,9 +868,9 @@ def assertIsInstance(obj, cls, msg=''):
"""Test that obj is an instance of cls
(which can be a class or a tuple of classes,
as supported by isinstance())."""
assert isinstance(obj, cls), (
"%sExpected object to be of type %r, found %r instead" % (
msg, cls, type(obj)))
if not isinstance(obj, cls):
err_msg = "{0}Expected type {1}, found {2} instead"
raise AssertionError(err_msg.format(msg, cls, type(obj)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need our own here? Nosetests has a pretty reasonable one:

    def assertIsInstance(self, obj, cls, msg=None):
        """Same as self.assertTrue(isinstance(obj, cls)), with a nicer
        default message."""
        if not isinstance(obj, cls):
            standardMsg = '%s is not an instance of %r' % (safe_repr(obj), cls)
            self.fail(self._formatMessage(msg, standardMsg))

(...although not as reasonable as pytest...)

Copy link
Contributor

Choose a reason for hiding this comment

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

we define everything already so you can either do: self.assertIsInstance or tm.assertIsInstance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, makes sense.

General marketing of pytest: in pytest you can just do assert isinstance(x, x), and it writes the error message, which is all nice and pythonic

Copy link
Contributor

Choose a reason for hiding this comment

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

@MaximilianR of course everyone likes pytest better. I have used it for years. But changing would be a bear, not sure if its worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah.
How dependent is the testing suite on nosetests, vs just unittest?

We were using nosetests, moved to pytest, and it worked really seamlessly. But that's because we were really only using nosetests as the runner, rather than any nose-specific features

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would work right away. But, the power if pytest is really using fixtures, parameterized tests, and clean looking asserts.

So need to convert most setUp/tearDown stuff to fixtures (a lot). properly use fixture scope (e.g. db stuff which you only want to create a database once per test run; this is the most complicated).

Parameterized tests are awesome, but again, need to restructure tests a bit for that.

I suppose the actual assert mechanism could be changed, but a lot of care would be needed.

And here's the rub, I think we would have to simply switch, then start changing things as its not back-compat to nose.

Given all this I am not optomistic this could be done quickly/easily. The upside is nice & clean tests, but getting there is hard.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would work right away. But, the power if pytest is really using fixtures, parameterized tests, and clean looking asserts.
And here's the rub, I think we would have to simply switch, then start changing things as its not back-compat to nose.

So what's in the way of making the change to py.test and then in time move over to using its features? There's no need to move everything over immediately?

Copy link
Contributor

Choose a reason for hiding this comment

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

nothing really. If you want to investigate actually seeing what would be needed to be changed to simply run it (and might involved a py.test compat plugin), then would be willing to switch I think. Though of course would be nice to see what is involved in adding these things: fixtures, assertion nomenclature, parameterization (e.g. maybe do one test file to see what its like).

This was discussed a little bit on the mailing list, but I don't think an issue exists for it.



def assert_isinstance(obj, class_type_or_tuple, msg=''):
Expand All @@ -882,9 +882,9 @@ def assertNotIsInstance(obj, cls, msg=''):
"""Test that obj is not an instance of cls
(which can be a class or a tuple of classes,
as supported by isinstance())."""
assert not isinstance(obj, cls), (
"%sExpected object to be of type %r, found %r instead" % (
msg, cls, type(obj)))
if isinstance(obj, cls):
err_msg = "{0}Input must not be type {1}"
raise AssertionError(err_msg.format(msg, cls))


def assert_categorical_equal(res, exp):
Expand Down