MNT: Merge locally defined test marks #23045

oscargus · 2022-05-14T10:03:13Z

PR Summary

Identical test marks were defined in multiple files. This moves them to testing.marks (for consistency, even if they were only defined in a single file).

Also deprecates matplotlib.checkdep_usetex.

Closes #14455

PR Checklist

Tests and Styling

[N/A] Has pytest style unit tests (and pytest passes).
[N/A] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

[N/A] New features are documented, with examples if plot related.
[N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
[N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
[N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

oscargus · 2022-05-14T12:26:42Z

Depending on the merge order of this and #23033 I will update the one that is not merged first.

greglucas · 2022-05-15T14:19:24Z

lib/matplotlib/testing/decorators.py

+needs_ghostscript = pytest.mark.skipif(
+    "eps" not in matplotlib.testing.compare.converter,
+    reason="This test needs a ghostscript installation")


Rather than a decorator, should these be turned into a "marker"? Then you'd decorate the tests with:
@pytest.mark.needs_lualatex and avoid the decorator imports.

How exactly would that work? You'd evaluate whether gostscript is present at test startup and then modify the marker selection? I don't know how to do that but I guess that's possible. However, importing the decorator seems much simpler.

I wasn't sure how easy it would be myself, just figured it would be worth the thought :) I think we could intercept the setup of the test and say something like:

if needs_ghostscript in item.markers and "eps" not in matplotlib.testing.compare.converter: pytest.skip(reason=...)

https://docs.pytest.org/en/6.2.x/example/markers.html#marking-platform-specific-tests-with-pytest

I agree though, that might be more than necessary here.

greglucas · 2022-05-15T21:22:14Z

lib/matplotlib/testing/decorators.py

+needs_ghostscript = pytest.mark.skipif(
+    "eps" not in matplotlib.testing.compare.converter,
+    reason="This test needs a ghostscript installation")


I wasn't sure how easy it would be myself, just figured it would be worth the thought :) I think we could intercept the setup of the test and say something like:

if needs_ghostscript in item.markers and "eps" not in matplotlib.testing.compare.converter: pytest.skip(reason=...)

https://docs.pytest.org/en/6.2.x/example/markers.html#marking-platform-specific-tests-with-pytest

I agree though, that might be more than necessary here.

greglucas · 2022-05-15T21:27:35Z

I think the doc failure is relevant here?

WARNING: autodoc: failed to import module 'decorators' from module 'matplotlib.testing'; the following exception was raised:
No module named 'pytest'

oscargus · 2022-05-17T09:03:09Z

I guess that there are a few solutions to the doc-build problem:

Add pytest to the doc-build requirements
Make the definitions of the decorators dependent on pytest being installed (and therefore test if it can be imported first).
Add them to tests/conftest.py or testing/conftest.py (and import them in tests/conftest.py) instead
Add them to a new _decorators.py file (possibly importing them in tests/conftest.py)

Any preference?

"Problem" is: https://matplotlib.org/stable/api/testing_api.html

tacaswell · 2022-05-17T16:26:50Z

I remember trying to do this a while ago, hitting this docs problem and then giving up on the effort :-p

I'm between 1 and "just accept the duplication".

anntzer · 2022-05-17T16:39:11Z

I think you can actually define these using unittest.skipIf instead; I believe pytest understands these and handles them just fine. If I'm correct, this way there's no need to add a dependency.

tacaswell · 2022-05-17T19:13:47Z

I think you can actually define these using unittest.skipIf instead;

That sounds brittle and works until it does not in confusing ways....

Given that we require scipy, a pretty complete latex, and graphviz to build the docs, adding pytest is 🤷🏻 .

anntzer · 2022-05-17T19:19:15Z

That sounds brittle and works until it does not in confusing ways....

Why? This is explicitly documented as working: https://docs.pytest.org/en/6.2.x/unittest.html
Also we don't depend on scipy anymore for the docs.

tacaswell · 2022-05-17T20:00:58Z

Why? This is explicitly documented as working

Fair, I still have concerns, but if it works, lets go with that plan.

Also we don't depend on scipy

Fair, but the point still stands for the other heavy dependencies and one more (pure-python) dependency is not too onerous.

anntzer · 2022-05-17T20:13:53Z

Note that this also means making matplotlib.testing.decorators not importable if pytest is not present (do you want to wrap the import in a try... except to reraise a more explicit exception?). Nothing undoable (and I doubt anyone is really importing that module for anything but doing pytest-related things), but I always feel like this slightly muddies the picture.
OTOH I certainly don't feel so strongly about this to block adding a dependency on pytest for this either.

timhoffm · 2022-05-22T07:39:25Z

-1 on adding a dependency on pytest.

-0.3 on using unittest. That's a working but non-standard solution. If we do that, it needs explanation.

Can we put the decorators in matplotlib.tests.__init__.py?

QuLogic · 2022-05-25T06:30:08Z

I wonder how this meshes with #14455? Or is that one now outdated?

oscargus · 2022-05-25T07:53:57Z

I wonder how this meshes with #14455?

I can try and deprecate mpl.checkdep_usetex as part of this as well. As I understand it, there were some fundamental problem in that approach (and it needs quite a bit of rebasing as well).

I'll find a new place for the remaining decorators.

timhoffm · 2022-05-26T07:32:34Z

Thinking again, it would be ok to have the decorators in testing if that does not introduce an import-time dependency on pytest. So some variant of try … expect ImportError or local imports could be a solution.

oscargus · 2022-05-26T12:34:19Z

It struck me that these are actually not decorators, but marks. So makes sense to add a file marks.py for this.

I also deprecated checkdep_usetex so #14455 can be closed.

timhoffm · 2022-05-26T22:37:06Z

lib/matplotlib/testing/marks.py

+    return True
+
+
+needs_ghostscript = pytest.mark.skipif(


By putting them here, we're technically making them public: matplotlib.testing.marks.needs_ghostscript.

While I'm not sure whether testing itself should be public. Let's not add to it. Either _marks.py or prefix all the markers. I'd go with the first, similar to _api.

Oh, and pytest calls these "markers". so, _markers.py it is.

Not critical here, but I still don't think these are technically markers because they aren't in the pytest.mark namespace, which means someone can't skip these from the command line with something like: pytest -m "not latex".

I was also thinking about that after reading up a bit further. On the other hand, I do not think you can do pytest -m "not skipif" either? So marker seems to have a rather wide interpretation.

Turns out you can actually do that, pytest -m skipif.

Should I rename it to _decorators.py then?

I wouldn't. I think it would actually be more confusing to have both a _decorators.py and decorators.py file in the same directory.

I guess one can add them as proper markers, but I had a quick look and it seemed to require quite a bit more effort than possibly worth it. As far as I can tell one would need to create a proper conftest.py with functions checking the markers and so on, not simply "inserting" a named marker derived from another marker. But I could have been bad at searching.

(Which is basically what you stated in an earlier comment.)

lib/matplotlib/testing/marks.py

anntzer · 2022-06-11T09:36:13Z

lib/matplotlib/testing/_markers.py

+needs_ghostscript = pytest.mark.skipif(
+    "eps" not in matplotlib.testing.compare.converter,
+    reason="This test needs a ghostscript installation")
+needs_lualatex = pytest.mark.skipif(


@oscargus Sorry I missed this before merging, but these names seem not so optimal now? needs_{lua,pdf,xe}latex now check both for the tex engine, and for the presence of pgf (this was less of an issue when the marks were local to test_backend_pgf. Perhaps rename them to needs_{lua,pdf,xe}tex_pgf? (Or just move them back to being local to the pgf backend test suite.)

oscargus added the Maintenance label May 14, 2022

oscargus mentioned this pull request May 14, 2022

Fix issue with tex-encoding on non-Unicode platforms #23033

Merged

2 tasks

oscargus changed the title ~~MNT: Move locally defined test decorators~~ MNT: Merge locally defined test decorators May 15, 2022

greglucas reviewed May 15, 2022

View reviewed changes

timhoffm approved these changes May 15, 2022

View reviewed changes

greglucas approved these changes May 15, 2022

View reviewed changes

tacaswell added this to the v3.6.0 milestone May 17, 2022

oscargus force-pushed the movedecorators branch 2 times, most recently from 170db62 to e80df49 Compare May 26, 2022 12:25

oscargus changed the title ~~MNT: Merge locally defined test decorators~~ MNT: Merge locally defined test marks May 26, 2022

oscargus force-pushed the movedecorators branch 3 times, most recently from cc9fbbc to 8bd907f Compare May 26, 2022 12:32

oscargus force-pushed the movedecorators branch from 8bd907f to fa59daf Compare May 26, 2022 12:37

timhoffm reviewed May 26, 2022

View reviewed changes

MNT: Move locally defined test marks

3154eb0

oscargus force-pushed the movedecorators branch from fa59daf to 3154eb0 Compare May 27, 2022 11:59

jklymak requested a review from timhoffm June 2, 2022 14:40

timhoffm approved these changes Jun 10, 2022

View reviewed changes

timhoffm merged commit 1e22895 into matplotlib:main Jun 10, 2022

oscargus deleted the movedecorators branch June 11, 2022 09:08

anntzer reviewed Jun 11, 2022

View reviewed changes

oscargus mentioned this pull request Jun 11, 2022

Rename test markers #23248

Merged

6 tasks

Uh oh!

MNT: Merge locally defined test marks #23045

MNT: Merge locally defined test marks #23045

Uh oh!

Conversation

oscargus commented May 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

oscargus commented May 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglucas commented May 15, 2022

Uh oh!

oscargus commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented May 17, 2022

Uh oh!

anntzer commented May 17, 2022

Uh oh!

tacaswell commented May 17, 2022

Uh oh!

anntzer commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented May 17, 2022

Uh oh!

anntzer commented May 17, 2022

Uh oh!

timhoffm commented May 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuLogic commented May 25, 2022

Uh oh!

oscargus commented May 25, 2022

Uh oh!

timhoffm commented May 26, 2022

Uh oh!

oscargus commented May 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oscargus commented May 14, 2022 •

edited

Loading

oscargus commented May 17, 2022 •

edited

Loading

anntzer commented May 17, 2022 •

edited

Loading

timhoffm commented May 22, 2022 •

edited

Loading