Add tests for the external process calls#8504
Add tests for the external process calls#8504tomspur wants to merge 1 commit intomatplotlib:masterfrom
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 |
QuLogic
left a comment
QuLogic
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
Same thing about the try/finally here.
tomspur
force-pushed
the
subprocesses
branch
2 times, most recently
from
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.
We don't really need to introduce randomness here, do we?
There was a problem hiding this comment.
Else where we use np.random.seed(19680801) as the seed to reduce randomness.
There was a problem hiding this comment.
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()) |
|
'power-cycled' to re-run against current master. |
| import pytest | ||
|
|
||
| import numpy as np | ||
| import matplotlib |
There was a problem hiding this comment.
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.
This needs to be de-dented. The savefig raises and this code never gets run
tacaswell
left a comment
tacaswell
left a comment
There was a problem hiding this comment.
Not all of the test code is actually run. Anyone can dismiss this review.
QuLogic
left a comment
QuLogic
left a comment
There was a problem hiding this comment.
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. |
tacaswell
modified the milestones:
2.1 (next point release),
2.2 (next next feature release)
|
Thanks @QuLogic for the rebase and cleanup! |
4 participants
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
dvipngstep.