Creating @skipif_font_missing

[PaMeirelles]

PR summary

Creates skipif_font_missing decorator to make tests involving fonts cleaner. Closes #27121

PR checklist

• "closes #0000" is in the body of the PR description to link the related issue
• N/A new and changed code is tested
• N/A Plotting related features are demonstrated in an example
• N/A New Features and API Changes are noted with a directive and release note
• N/A Documentation complies with general and docstring guidelines

[ksunden]

I think my inclination is to implement this more like we do for tex library dependencies, where we have a boolean flag returning method and just combine that with pytest.mark.skipif/pytest.skip.

This additionally provides flexibility to xfail instead of skip, and handles the case of skipping inside of the method so that parameterized versions still run, etc.

While I like the conciseness of the decorator, I think it is lacking in that flexibility.

See:

matplotlib/lib/matplotlib/testing/__init__.py

Lines 174 to 179 in 0f61895

	def _has_tex_package(package):
	try:
	mpl.dviread.find_tex_file(f"{package}.sty")
	return True
	except FileNotFoundError:
	return False

For the prior art of what we do for tex

[ksunden]

Should probably use functools.wraps here, as we do not wish to assume the signature of the wrapped function

[ksunden]

I don't like that this will skip both parameterized versions of this test if only one of the fonts is missing... The implementation where it is checked in the method is more verbose, but allows for these parameterizations to still test when only some fonts are available.

[story645]

Thanks for taking this on! I agree w/ @ksunden on retaining the current behavior such that:

• @skipif_font_missing(['A', 'B']) parametrizes two tests:
  • test a: runs if font A is present
  • test b: runs if font B is present
• @skipif_font_missing([['A', 'B'], ['C', 'D']]) also parameterizes two tests:
  • test a: run if A and B are present
  • test b: run if C and D are present

But I would prefer that this provides a font_list fixture so that there's no chance of skipped files being out of sync with tested files - basically I don't want to risk the setup where the paramterized font_list is different from the tested font_list & I can easily see that happening accidentally cause of thrashing.

Could the way to do that while still having true false semantics is that if a font_file is not found then it returns false, and if the file is found it returns the file (or true)-> basically there's the "bool check if fontfound" function that does what Kyle is asking for, but then it's wrapped in the @skip_if decorator I'm asking for? So that the different functions can be used as needed?

ETA: something like:

def _font_found(font_list):
    # code to look up fonts      
    return is_found

def skipif_font_found(font_list):
    _font_found(font_list)
   #does whatever magic to be a fixture

[tacaswell]

Writing decorators that work with pytest is a bit non-trivial because of the amount of magic that pytest does to make discovery and fixtures work.

I think you either want to parameterize using pytest.mark like

matplotlib/lib/matplotlib/testing/decorators.py

Lines 46 to 53 in 1d1171f

	def _checked_on_freetype_version(required_freetype_version):
	import pytest
	return pytest.mark.xfail(
	not _check_freetype_version(required_freetype_version),
	reason=f"Mismatched version of freetype. "
	f"Test requires '{required_freetype_version}', "
	f"you have '{ft2font.__freetype_version__}'",
	raises=ImageComparisonFailure, strict=False)

or you need to do things like

matplotlib/lib/matplotlib/testing/decorators.py

Lines 190 to 203 in 1d1171f

	def decorator(func):
	old_sig = inspect.signature(func)
	@functools.wraps(func)
	@pytest.mark.parametrize('extension', extensions)
	@matplotlib.style.context(style)
	@_checked_on_freetype_version(freetype_version)
	@functools.wraps(func)
	def wrapper(*args, extension, request, **kwargs):
	__tracebackhide__ = True
	if 'extension' in old_sig.parameters:
	kwargs['extension'] = extension
	if 'request' in old_sig.parameters:
	kwargs['request'] = request

Because the tests are already parameterized I think you will have to do something closer to the second.

Overall I am 👎🏻 on this change. It technically reduces a bit of duplication, but the balance of complexity we have to pick up (writing a decorator that integrates with pytest's fixtures and test discovery and maybe a second one for non-parameterized tests) to do so is not worth it.

[story645]

It technically reduces a bit of duplication,

It's not just a bit, we use this check in a lot of the font tests. The reason I want this is to improve consistency in the tests. I had to add these tests/fixtures in a bunch of places b/c the code/tests segfault if the font is missing - which yes we should definitely be writing more robust code on the C side so that doesn't happen- and am far more comfortable with a consistent way of doing so.

[tacaswell]

segfault if the font is missing

You can segfault main with a missing font?! If so is there an issue for that and it should be release critical for 3.8.1

[story645]

You can segfault main with a missing font?! If so is there an issue for that and it should be release critical for 3.8.1

Yeah... any font code where you're trying to access a property on face fails if face is none...the Python code guards against this but not the C++ code.

[ksunden]

Do we have a reproducer that segfaults?

If you pass a non-existant font, you still just get:

findfont: Font family 'this_doesnt_exist' not found.

which isn't a segfault.

[story645]

I'm not gonna be near a computer for most of today, but try Ft2Font objects directly - that findfont warning is printed in fontmanager, & the problems can arise when you bypass font-manager.

[ksunden]

I mean, you can't make a FT2Font object without a valid font file, so I don't think there is a problem...

[story645]

Maybe, but lots of the c++ font code isn't super well tested (xref #27115) which is why I could remove warnings without breaking tests.

And also you can totally pass in a missing fontfile in as part of the fallback list - that's how I get the the fallback Chinese examples to work on Windows. https://matplotlib.org/devdocs/users/explain/text/fonts.html#font-fallback

[story645]

@PaMeirelles I think what objection there is is to the decorator, so I'd recommend factoring out a Boolean "_is_font_found" method that could be used in a decorator or tests or wherever else. Possibly this method can be put in font manager or book or something - @anntzer may have an idea.

Then we can separately discuss the utility of wrapping it in a decorator.

[anntzer]

I guess we really just need a helper in font_manager that does basically the same thing as findfont, but returns None (or raises) when the font of the requested family is not found, without ever trying to return a font from another family or performing fallback?
I agree with @tacaswell that wrapping in a pytest decorator is just too much complexity to be worth it.

[story645]

I guess we really just need a helper in font_manager that does basically the same thing as findfont, but returns None (or raises) when the font of the requested family is not found, without ever trying to return a font from another family or performing fallback?

Yeah, specifically it can't raise & should be passable to skip_if

And there's probably a way to compose it with a font_list parameterization to do what I want

[melissawm]

hi @PaMeirelles - let us know if you need help with rebasing this. Here's basic instructions: https://matplotlib.org/devdocs/devel/development_workflow.html#rebase-onto-upstream-main

[tacaswell]

Thank you for your work on this @PaMeirelles but I am going to close this as it turned out to be more complex than anticipated.

I hope we hear from you again!