TST: Skipif decorator for matplotlib #18190 #18427

WillAyd · 2017-11-22T14:33:02Z

progress towards TST: make _skip_if into pytest decorators #18190
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff

The behavior here is slightly different than before when the function was used at the module level. Previously, an entire module being skipped would count as one skip entry. Now with the decorator, each test case within the module gets counted as a skip.

codecov · 2017-11-22T18:02:39Z

Codecov Report

Merging #18427 into master will decrease coverage by 0.01%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master   #18427      +/-   ##
==========================================
- Coverage   91.35%   91.33%   -0.02%     
==========================================
  Files         163      164       +1     
  Lines       49714    49735      +21     
==========================================
+ Hits        45415    45426      +11     
- Misses       4299     4309      +10

Flag	Coverage Δ
#multiple	`89.13% <95.23%> (ø)`	⬆️
#single	`39.64% <52.38%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/util/testing.py	`100% <ø> (ø)`	⬆️
pandas/util/_test_decorators.py	`95.23% <95.23%> (ø)`
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.8% <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 d421a09...fb9c6e1. Read the comment docs.

codecov · 2017-11-22T18:02:43Z

Codecov Report

Merging #18427 into master will increase coverage by <.01%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master   #18427      +/-   ##
==========================================
+ Coverage   91.32%   91.33%   +<.01%     
==========================================
  Files         163      164       +1     
  Lines       49798    49819      +21     
==========================================
+ Hits        45479    45503      +24     
+ Misses       4319     4316       -3

Flag	Coverage Δ
#multiple	`89.13% <95.23%> (+0.02%)`	⬆️
#single	`40.8% <52.38%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/util/testing.py	`100% <ø> (ø)`	⬆️
pandas/util/_test_decorators.py	`95.23% <95.23%> (ø)`
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️
pandas/core/indexes/datetimes.py	`95.71% <0%> (+0.09%)`	⬆️
pandas/plotting/_converter.py	`65.25% <0%> (+1.81%)`	⬆️

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 262e8ff...ffcbe0a. Read the comment docs.

jreback · 2017-11-22T23:10:22Z

@TomAugspurger

TomAugspurger · 2017-11-26T12:51:05Z

pandas/tests/util/test_util.py

+
+
+def test__safe_import():
+    from _pytest.monkeypatch import MonkeyPatch


What is _pytest? On mobile and can’t check easily.

That reflects the package structure of pytest. However, I think this can be re-written another way as I'm reading more on how pytest plugins work. Let me re-push

https://holgerkrekel.net/2009/03/03/monkeypatching-in-unit-tests-done-right/

TomAugspurger · 2017-11-26T12:56:21Z

pandas/util/_test_decorators.py

+        import sys
+        version = getattr(sys.modules[mod_name], '__version__')
+        if version:
+            from pkg_resources import parse_version as pv


We typically use distutils but I think we require setup tools now so this is fine, right?

Yes there is currently an entry for setuptools in requirements_dev.txt FWIW this was inspired by the current importorskip implementation in pytest that these skip functions were using previously

TomAugspurger · 2017-11-26T13:03:11Z

I think if you merge in master, the CirclCI failure will go away.

jreback · 2017-11-26T15:40:31Z

pandas/util/_test_decorators.py

@@ -0,0 +1,45 @@
+import pytest
+


can you add some text as to the purpose of this module

jreback · 2017-11-26T15:41:09Z

pandas/util/_test_decorators.py

+        return mod
+    else:
+        import sys
+        version = getattr(sys.modules[mod_name], '__version__')


I would use LooseVersion here (as we use this elsewhere)

jreback · 2017-11-26T15:42:18Z

pandas/util/_test_decorators.py

+import pytest
+
+
+def _safe_import(mod_name, min_version=None):


this could replace pandas.util.testing.skip_if_no_package as well (or maybe be an input to it)

since this is already in a private module, you can name it safe_import

WillAyd · 2017-11-26T16:35:28Z

@jreback 3/4 of your suggested edits made. The only one I didn't do is replacing pandas.util.testing.skip_if_no_package. Looking at that function, it is almost always used when looking for scipy dependencies (also saw one check for tables). You are correct that the safe_import method should be able to replace this, but I'd rather tackle that with the PR that converts the scipy skip functions into decorators rather than this change with mpl.

jreback · 2017-11-26T16:45:30Z

totally fine to do later just bringing it up

jreback · 2017-11-27T11:39:33Z

pandas/tests/test_resample.py

@@ -13,6 +13,7 @@
 import pandas as pd
 import pandas.tseries.offsets as offsets
 import pandas.util.testing as tm
+from pandas.util._test_decorators import skip_if_no_mpl


can you write these as
import pandas.util._test_decorators as td

then use td.skip_if_no_mpl

I think a bit simpler

jreback · 2017-11-27T11:39:47Z

@TomAugspurger

TomAugspurger · 2017-11-27T17:51:26Z

pandas/util/_test_decorators.py

+        return True
+
+
+skip_if_no_mpl = pytest.mark.skipif(_skip_if_no_mpl(),


I recall some issues with custom markers (#16680).

Do you know if this will cause similar issues with the --strict flag? Could you try pytest --collect-only --strict and see if anything errors? We may be OK since it's a skipif marker, which pytest already knows about...

When running that locally I get one error that high_memory is not a registered markerbecause pandas/tests/io/parser/c_parser_only.py has @pytest.mark.high_memory on line 21, but the registered marker is just @pytest.mark.highmemory (no underscore). I believe that to be unrelated to the change here

Oh, OK. I'll make a PR for the high_memory issue. Thanks!

WillAyd force-pushed the pd-test-decs branch 2 times, most recently from 262eacf to fb9c6e1 Compare November 22, 2017 18:02

jreback added the Testing pandas testing functions or related to the test suite label Nov 22, 2017

TomAugspurger approved these changes Nov 26, 2017

View reviewed changes

WillAyd force-pushed the pd-test-decs branch 2 times, most recently from b63873d to 1e055fb Compare November 26, 2017 14:57

jreback requested changes Nov 26, 2017

View reviewed changes

WillAyd force-pushed the pd-test-decs branch from 1e055fb to 46a43d7 Compare November 26, 2017 16:30

WillAyd force-pushed the pd-test-decs branch 2 times, most recently from 13628df to 6d7ff3c Compare November 26, 2017 22:01

jreback reviewed Nov 27, 2017

View reviewed changes

TST: Skipif decorator for matplotlib pandas-dev#18190

ffcbe0a

WillAyd force-pushed the pd-test-decs branch from 6d7ff3c to ffcbe0a Compare November 27, 2017 14:53

TomAugspurger reviewed Nov 27, 2017

View reviewed changes

TomAugspurger merged commit 34b036c into pandas-dev:master Nov 27, 2017

WillAyd deleted the pd-test-decs branch November 27, 2017 18:18

TomAugspurger mentioned this pull request Nov 27, 2017

BUG: Fix marker for high memory #18526

Merged

WillAyd mentioned this pull request Dec 4, 2017

TST: make _skip_if into pytest decorators #18190

Closed

16 tasks

WillAyd mentioned this pull request Dec 13, 2017

pandas tests fail when matplotlib is headless #18513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST: Skipif decorator for matplotlib #18190 #18427

TST: Skipif decorator for matplotlib #18190 #18427

WillAyd commented Nov 22, 2017

codecov bot commented Nov 22, 2017

codecov bot commented Nov 22, 2017 •

edited

Loading

jreback commented Nov 22, 2017

TomAugspurger Nov 26, 2017

WillAyd Nov 26, 2017

TomAugspurger Nov 26, 2017

WillAyd Nov 26, 2017

TomAugspurger commented Nov 26, 2017

jreback Nov 26, 2017

jreback Nov 26, 2017

jreback Nov 26, 2017

jreback Nov 26, 2017

WillAyd commented Nov 26, 2017

jreback commented Nov 26, 2017

jreback Nov 27, 2017

jreback commented Nov 27, 2017

TomAugspurger Nov 27, 2017

WillAyd Nov 27, 2017 •

edited

Loading

TomAugspurger Nov 27, 2017



		def test__safe_import():
		from _pytest.monkeypatch import MonkeyPatch

		return True


		skip_if_no_mpl = pytest.mark.skipif(_skip_if_no_mpl(),

TST: Skipif decorator for matplotlib #18190 #18427

TST: Skipif decorator for matplotlib #18190 #18427

Conversation

WillAyd commented Nov 22, 2017

codecov bot commented Nov 22, 2017

Codecov Report

codecov bot commented Nov 22, 2017 • edited Loading

Codecov Report

jreback commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Nov 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Nov 26, 2017

jreback commented Nov 26, 2017

Choose a reason for hiding this comment

jreback commented Nov 27, 2017

Choose a reason for hiding this comment

WillAyd Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2017 •

edited

Loading

WillAyd Nov 27, 2017 •

edited

Loading