Skip to content

update testing helpers #16990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 18, 2020
Merged

update testing helpers #16990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions lib/matplotlib/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1168,29 +1168,29 @@ def _init_tests():
"Freetype build type is {}local".format(
"" if ft2font.__freetype_build_type__ == 'local' else "not "))

try:
import pytest
except ImportError:
print("matplotlib.test requires pytest to run.")
raise


@cbook._delete_parameter("3.2", "switch_backend_warn")
@cbook._delete_parameter("3.3", "recursionlimit")
def test(verbosity=None, coverage=False, switch_backend_warn=True,
recursionlimit=0, **kwargs):
"""Run the matplotlib test suite."""
_init_tests()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But now you don't make the warning about FreeType. Why did the pytest import check have to move into this function and not just stay in _init_tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do still get the import warning about FreeType because the pytest configure calls _init_tests

matplotlib/lib/matplotlib/testing/conftest.py

Line 25 in 348e9fe

matplotlib._init_tests()

As originally written the _init_tests will be called twice: Once before the logger is set up (here), and then again by pytest (linked above) which by then logging is being captured. I didn't believe _init_tests needed to be called here because it doesn't raise an exception and you're not going to see the logs since they aren't set up at this point. If the warning in _init_tests isn't actually a warning it should be emitted in a way that a user can see it (currently it isn't).

The reason I moved the pytest import into this function is so this function doesn't need to blindly capture ImportError and can return -1 for pytest not being installed. This is all detailed in the PR description if you would like to read that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QuLogic thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QuLogic Does this satisfy your question? I've put this on the call agenda for tomorrow, but if you're ok with this, we don't have to spend time on it.


try:
import pytest
except ImportError:
print("matplotlib.test requires pytest to run.")
return -1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am skeptical about the API change, but this function is already returning an error code so any users of this will likely already be expecting to handle error codes so I expect it will do little harm.

Copy link
Contributor Author

@terencehonles terencehonles May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was basically my position. It wasn't obvious to me why this was "broken" and I thought it wasn't used anymore. It was, but I didn't have the right environment. I then sought to clarify the whole process until getting pytest to run. I was contemplating keeping the same API for this part, but the code would be less clear and as you noted the test function will already return error codes on pytest errors


if not os.path.isdir(os.path.join(os.path.dirname(__file__), 'tests')):
raise ImportError("Matplotlib test data is not installed")
print("Matplotlib test data is not installed")
return -1

old_backend = get_backend()
old_recursionlimit = sys.getrecursionlimit()
try:
use('agg')
if recursionlimit:
sys.setrecursionlimit(recursionlimit)
import pytest

args = kwargs.pop('argv', [])
provide_default_modules = True
Expand Down
6 changes: 3 additions & 3 deletions lib/matplotlib/testing/decorators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,11 @@ def _raise_on_image_difference(expected, actual, tol):

err = compare_images(expected, actual, tol, in_decorator=True)
if err:
for key in ["actual", "expected"]:
for key in ["actual", "expected", "diff"]:
err[key] = os.path.relpath(err[key])
raise ImageComparisonFailure(
'images not close (RMS %(rms).3f):\n\t%(actual)s\n\t%(expected)s '
% err)
('images not close (RMS %(rms).3f):'
'\n\t%(actual)s\n\t%(expected)s\n\t%(diff)s') % err)


def _skip_if_format_is_uncomparable(extension):
Expand Down
7 changes: 6 additions & 1 deletion tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@


if __name__ == '__main__':
from matplotlib import test
try:
from matplotlib import test
except ImportError:
print('matplotlib.test could not be imported.\n\n'
'Try a virtual env and `pip install -e .`')
sys.exit(-1)

parser = argparse.ArgumentParser(add_help=False)
parser.add_argument('--recursionlimit', type=int, default=None,
Expand Down