CLN: Split up Boolean array tests #32780
Conversation
dsaxton
changed the title
| return pd.array(make_data(), dtype=dtype) | ||
|
|
||
|
|
||
| class TestComparisonOps(BaseOpsUtil): |
There was a problem hiding this comment.
should these be parametrized over EA/Series/DataFrame? (and eventually ExtensionIndex)?
There was a problem hiding this comment.
Perhaps, I'm trying to avoid making any actual logic changes to tests here though since the PR is already quite big
| from pandas.tests.extension.base import BaseOpsUtil | ||
|
|
||
|
|
||
| class TestLogicalOps(BaseOpsUtil): |
There was a problem hiding this comment.
Side comment: do we need these classes in general? Seems like the location / name of the test should give enough context
There was a problem hiding this comment.
In general, if there is only a single class in a file, certainly not. But in this case, it might use things from the BaseOpsUtil class
There was a problem hiding this comment.
agreed with @jorisvandenbossche. Largely its a matter of personal preference; I tend to retain the class structure in cases where it helps further organize tests
|
Personally, I don't find that this makes it easier to find the tests, though ... (with this I would need to look in multiple files of which I don't know the name) Now, I assume that if we keep adding tests here, splitting it becomes unavoidable. The question might then also be: where to put the line on what should be tested here? |
| from pandas.tests.extension.base import BaseOpsUtil | ||
|
|
||
|
|
||
| class TestLogicalOps(BaseOpsUtil): |
There was a problem hiding this comment.
In general, if there is only a single class in a file, certainly not. But in this case, it might use things from the BaseOpsUtil class
| import pandas as pd | ||
| import pandas._testing as tm | ||
|
|
||
|
|
There was a problem hiding this comment.
We could also still combine the different ops in a single file?
simonjayhawkins
added
the
Testing
Yes, if that was unclear: if the arithmetic/reduction tests can/should be parametrized over EA/Series/DataFrame, then they likely should be moved to tests.arithmetic or tests.reductions etc. (the situation is slightly complicated by the fact that those tests are also parametrized over Index which these can't be until ExtensionIndex is in operation) |
|
Curious if @jreback has any preferences on the layout of these tests? |
|
once a file gets > 500 lines, splitting into sub-modules is really needed. we have already split most other tests, so this is fine. |
Apologies for the size of this PR, but it looks like the ExtensionArray tests sometimes have their own directories by type, and other times all tests for the type are in a single file (e.g., https://github.com/pandas-dev/pandas/tree/master/pandas/tests/arrays/categorical vs. https://github.com/pandas-dev/pandas/blob/master/pandas/tests/arrays/test_boolean.py). I think the "whole directory for type" structure makes it easier to find and add new tests, so I'm doing that here for the Boolean tests. There shouldn't be any changes other than moving things around.