TST/CLN: parametrize coercion tests

[WillAyd]

• progress towards CLN: Refactor test_coercion.py to Leverage Parametrization #18706
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

This is not yet finished but wanted to share progress in case of feedback. The main thing I'm questioning is the need to use the test_has_comprehensive_tests method in CoercionBase. If we want to keep I would need to refactor, but I'm curious if others even find it necessary given that there are often just blank tests being created in subclasses to make that test pass

[pep8speaks]

Hello @WillAyd! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 12, 2017 at 15:34 Hours UTC

[jreback]

generic approach looks good.

no problem removing the actual has_comprehensive test. just use a fixture for the dtypes and the klasses and should have it covered (by-definition then we will test everything).

[WillAyd]

As far as the fixture goes are you still looking for something to inspect the test methods being provided? Starting down that path but want to make sure I'm not over-engineering a solution.

In simple pseudo-code, I have a fixture that looks like:

@pytest.fixture(autouse=True, scope='class')
def check_coverage(request):
    cls = request.cls
    # Check class metadata

With that I was planning to inspect each method and its parametrization to ensure all dtypes and klasses have been accounted for. Before it was pretty simple because there was a consistent naming pattern, but with parametrization the method naming isn't going to be as consistent. In some instances, the distinction of whether we are using a pd.Index or a pd.Series would be visible in the method name, but in some other cases a mark would determine which object to use.

[jreback]

i think that’s overkill

you can specify ids= if you need in the fixture to have consistent naming

[codecov]

Codecov Report

Merging #18721 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18721      +/-   ##
==========================================
+ Coverage   91.59%   91.59%   +<.01%     
==========================================
  Files         153      153              
  Lines       51364    51317      -47     
==========================================
- Hits        47046    47004      -42     
+ Misses       4318     4313       -5

Flag	Coverage Δ
#multiple	89.45% <ø> (+0.01%)	⬆️
#single	40.71% <ø> (-0.15%)	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	25% <0%> (-58.34%)	⬇️
pandas/util/_test_decorators.py	93.33% <0%> (-0.79%)	⬇️
pandas/util/testing.py	82.01% <0%> (-0.52%)	⬇️
pandas/core/indexes/category.py	97.2% <0%> (-0.31%)	⬇️
pandas/io/formats/format.py	96.03% <0%> (-0.15%)	⬇️
pandas/core/dtypes/dtypes.py	95.14% <0%> (-0.14%)	⬇️
pandas/core/frame.py	97.81% <0%> (-0.1%)	⬇️
pandas/core/indexes/timedeltas.py	91.21% <0%> (-0.06%)	⬇️
pandas/core/indexes/numeric.py	97.33% <0%> (-0.04%)	⬇️
pandas/core/indexes/period.py	92.9% <0%> (-0.04%)	⬇️
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96439fb...7a072ac. Read the comment docs.

[codecov]

Codecov Report

Merging #18721 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18721      +/-   ##
==========================================
- Coverage   91.59%   91.57%   -0.03%     
==========================================
  Files         153      153              
  Lines       51364    51364              
==========================================
- Hits        47046    47035      -11     
- Misses       4318     4329      +11

Flag	Coverage Δ
#multiple	89.43% <ø> (-0.01%)	⬇️
#single	40.74% <ø> (-0.12%)	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	25% <0%> (-58.34%)	⬇️
pandas/util/testing.py	82.34% <0%> (-0.2%)	⬇️
pandas/core/frame.py	97.81% <0%> (-0.1%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96439fb...7a072ac. Read the comment docs.

[jreback]

where is this used?

[WillAyd]

The autouse and scope args make it so that it is used by every class within the module

[jreback]

oh, ok, cool. thanks for this patch. Nice work!

[jbrockmendel]

@WillAyd This is neat. I'm wondering: what would it take to put something like this together to check for permutations of arithmetic operations and operands? I don't quite grok pytest's namespacing, in particular where cls.klasses, cls.dtypes, cls.method come from and what request.node.session.items corresponds to.

Update the klasses/dtypes/method are a bit more clear now that I look at the whole file and not just the diff.

[WillAyd]

cls.klasses, cls.dtypes and cls.method don't have anything to do with pytest - they are all class variables built into the tests in this module. I was inspired by this link on how to initially set this up, but had to tweak slightly given that link has a session-scoped fixture whereas here we are working with a class-level scope.

Basically request.node.session.items traverses from the fixture to the node (pandas.tests.io.indexing.test_coercion::TestFoo) and then goes from the node to the session. The session contains all of the test items, so iterating over them this code checks if all of the klasses, dtypes and method combinations set in the "in-scope" class are defined somewhere in the suite.

I don't entirely understand what you are trying to do with operations and operands but assuming you wanted to set up those combinations within a parametrization fixture I believe you could access that metadata by looking at .callspec.params on each object in request.node.session.items.

[jbrockmendel]

I don't entirely understand what you are trying to do with operations and operands

I going through #18824 I'm finding lots of cases that are not tested, saw this bit of code and thought it might be possible to enumerate e.g. op = [__add__, __sub__, ...], vec_classes = [Series, DatetimeIndex, np.ndarray, ...], scalar_types = [...], null_types=[...], right = [...] and check that all the cases are tested.

That would also be helpful because there are a ton of cases where tests are duplicated because test_foo and test_bar both test foo+bar, bar+foo. Not that this is a big problem, but it'd be nice to be systematic about it.