Skip to content

BUG: broken inheritance within pytest? #22211

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

Closed
h-vetinari opened this issue Aug 6, 2018 · 2 comments
Closed

BUG: broken inheritance within pytest? #22211

h-vetinari opened this issue Aug 6, 2018 · 2 comments

Comments

@h-vetinari
Copy link
Contributor

This is something I discovered while responding to feedback for #21902

There's a lot going on tests/indexes/common.py, and to skip tests that cannot work, several tests have an if condition to exclude certain cases. In particular, test_has_duplicates (currently still named ambiguously as test_duplicates) has a check

if type(indices) is not self._holder:
    pytest.skip(...)

which I wanted to copy to test_duplicated.

@jreback asked my why this isn't if not isinstance(indices, self._holder), but adding this caused errors. Upon some investigating, it turns out that the isinstance-check with Int64Index yields the wrong result within pytest. In a normal interpreter, the answers are as expected:

import pandas as pd
pd.__version__
# 0.24.0.dev0+422.gcfa618249.dirty
isinstance(pd.DatetimeIndex, pd.Int64Index)
# False
isinstance(pd.PeriodIndex, pd.Int64Index)
# False
isinstance(pd.TimedeltaIndex, pd.Int64Index)
# False

But changing test_has_duplicates as follows shows that this is not the same in pytest:

    def test_has_duplicates(self, indices):
        if not isinstance(indices, self._holder):  # <- changed as requested
            pytest.skip('Can only check if we have the correct type')
        if not len(indices) or isinstance(indices, MultiIndex):
            # MultiIndex tested separately in:
            # tests/indexes/multi/test_unique_and_duplicates
            pytest.skip('Skip check for empty Index and MultiIndex')
        # the following check shows that inheritance is sometimes broken
        if (isinstance(indices, self._holder)
            and type(indices) is not self._holder
            and self._holder is not Index):
            # for legitimate inheritance the results of isinstance(A, B) and
            # type(A) is B will be different (this is why we exclude B=Index).
            # But this also catches cases where isinstance yields the wrong answer
            raise ValueError(f'type(A): {type(indices).__name__}, '
                             f'B: {self._holder.__name__}, isinstance(A, B): '
                             f'{isinstance(indices, self._holder)}, '
                             f'type(A) is B: {type(indices) is self._holder}')

        idx = self._holder([indices[0]] * 5)
        assert not idx.is_unique
        assert idx.has_duplicates

This yields the following output in a run of pytest pandas/tests/indexes:

================================== FAILURES ===================================
______________ TestInt64Index.test_has_duplicates[DatetimeIndex] ______________
[...]
E           ValueError: type(A): DatetimeIndex, B: Int64Index, isinstance(A, B): True, type(A) is B: False

_______________ TestInt64Index.test_has_duplicates[PeriodIndex] _______________
[...]
E           ValueError: type(A): PeriodIndex, B: Int64Index, isinstance(A, B): True, type(A) is B: False

_____________ TestInt64Index.test_has_duplicates[TimedeltaIndex] ______________
[...]
E           ValueError: type(A): TimedeltaIndex, B: Int64Index, isinstance(A, B): True, type(A) is B: False

====== 3 failed, 7686 passed, 1033 skipped, 10 xfailed in 122.11 seconds ======
@jbrockmendel
Copy link
Member

The first set of checks above should probably be “issubclass(pd.DatetimeIndex, pd.Int64Index)”, which would return True.

@h-vetinari
Copy link
Contributor Author

@jbrockmendel Ah, you are right of course. Got confused between instances and classes...

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

No branches or pull requests

2 participants