Skip to content

DISC: One big Extension Test class #54420

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
jbrockmendel opened this issue Aug 4, 2023 · 1 comment · Fixed by #54432
Closed

DISC: One big Extension Test class #54420

jbrockmendel opened this issue Aug 4, 2023 · 1 comment · Fixed by #54432
Labels
ExtensionArray Extending pandas with custom dtypes or arrays. Testing pandas testing functions or related to the test suite

Comments

@jbrockmendel
Copy link
Member

jbrockmendel commented Aug 4, 2023

I've been trying to improve the situation in tests/extension with the goals being

  1. Clean up some bad patterns we use for our own EAs (e.g. REF: avoid monkeypatch in arrow tests #54361)
  2. Change the base class patterns that led to the bad patterns from 1 (e.g. REF: dont pass exception to check_opname #54365)
  3. Get to a point where few/none of the tests need to be overridden, so authors just have to implement the relevant fixtures and maybe some hopefully-well-documented helpers.

One pain point I'm finding is that there are 23 test classes exposed in tests.extension.base and authors are supposed to subclass most of them, a few being optional (and some being mutually exclusive xref #44742). This introduces some failure modes:

  1. Failing to subclass all of them (or the desired subset)
    1a) e.g. in test_interval I see 14 test classes. I hope the others are excluded intentionally, but don't really know.
    1b) If we introduce a new test class (e.g. i think the BaseAccumulateTests is relatively recent) an existing EA might not know it needs updating
  2. Failing to subclass the right thing (e.g. in test_sparse we have TestComparisonOps that looks like it should subclass BaseComparisonOpsTests but doesn't, not immediately obvious if this is intentional)

I think it would be a better author/maintainer experience if we had all those test classes inherited into one TestExtensionArray class that authors (and our own EA tests) inherit. The downside is backward compatibility for existing 3rd party test suites. I guess we could continue to expose the separate classes, potentially deprecate them. The main trouble would be the mutually-exclusive ones.

Thoughts?

@jbrockmendel jbrockmendel added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Aug 4, 2023
@mroeschke
Copy link
Member

I think it would be a better author/maintainer experience if we had all those test classes inherited into one TestExtensionArray class that authors (and our own EA tests) inherit.

+1. It should hopefully be easier to track what parts are implemented with one class. If this route is chosen, I think the base class should have more property base testing and authors can make the assertions stricter

@lithomas1 lithomas1 added Testing pandas testing functions or related to the test suite ExtensionArray Extending pandas with custom dtypes or arrays. and removed Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ExtensionArray Extending pandas with custom dtypes or arrays. Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants