-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add tests for the external process calls #8504
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.
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
Conversation
Removing unused imports in unrelated files should probably go in its own PR. |
I removed the commits with the unused imports again. With these two new tests, the overage of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but tests could be simplified a bit with some pytest stuff.
|
||
|
||
@needs_tex | ||
def test_failing_latex(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the tmpdir
fixture to get a temporary directory here; shouldn't need tempfile
.
in output | ||
assert "Double subscript." in output | ||
raise | ||
os.remove(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't use the fixture, then this needs to go in a try
/finally
or it should use the TemporaryFile
context manager.
output = str(exc) | ||
assert "LaTeX was not able to process the following string:" \ | ||
in output | ||
assert "Double subscript." in output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extra try
/except
is not necessary; use the result of the pytest.raises
context manager to assert the message.
|
||
|
||
@needs_tex | ||
def test_failing_latex(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use the tmpdir
fixture here too.
with pytest.raises(RuntimeError): | ||
try: | ||
plt.savefig(path) | ||
except RuntimeError as exc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing about the try
/except
here.
in output | ||
assert "Double subscript." in output | ||
raise | ||
os.remove(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing about the try
/finally
here.
b587822
to
58d2e23
Compare
matplotlib.rcParams['text.usetex'] = True | ||
|
||
# This failes with "Double subscript" | ||
plt.xlabel("$%f_2_2$" % np.random.random()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need to introduce randomness here, do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Else where we use np.random.seed(19680801)
as the seed to reduce randomness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, but I'm not so sure this test even needs to be random.
matplotlib.rcParams['text.usetex'] = True | ||
|
||
# This failes with "Double subscript" | ||
plt.xlabel("$%f_2_2$" % np.random.random()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question?
'power-cycled' to re-run against current master. |
@@ -12,6 +12,7 @@ | |||
import pytest | |||
|
|||
import numpy as np | |||
import matplotlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed; rcParams
is directly imported on the next line.
plt.xlabel("$%f_2_2$" % np.random.random()) | ||
with pytest.raises(RuntimeError) as excinfo: | ||
plt.savefig(path) | ||
output = str(excinfo.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be de-dented. The savefig
raises and this code never gets run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all of the test code is actually run. Anyone can dismiss this review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See @tacaswell's last comment; the last line in the pytest.raises
context must be the one that raises.
Ping? This also needs a rebase. |
Thanks @QuLogic for the rebase and cleanup! |
This PR tries to add tests for #7572 and tests mainly the dvi and latex backend.
I could not find a way to test e.g. the png backend as this requires a working dvi in the first place, but would then fail in the
dvipng
step.