Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

tomspur
Copy link
Contributor

@tomspur tomspur commented Apr 17, 2017

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.

@QuLogic
Copy link
Member

QuLogic commented Apr 17, 2017

Removing unused imports in unrelated files should probably go in its own PR.

@tomspur
Copy link
Contributor Author

tomspur commented Apr 21, 2017

I removed the commits with the unused imports again.

With these two new tests, the overage of texmanager.py improved now at least by 0.65%

Copy link
Member

@QuLogic QuLogic left a 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():
Copy link
Member

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)
Copy link
Member

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
Copy link
Member

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():
Copy link
Member

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:
Copy link
Member

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)
Copy link
Member

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.

@dstansby dstansby added this to the 2.1 (next point release) milestone Apr 24, 2017
@tomspur tomspur force-pushed the subprocesses branch 2 times, most recently from b587822 to 58d2e23 Compare April 26, 2017 21:33
matplotlib.rcParams['text.usetex'] = True

# This failes with "Double subscript"
plt.xlabel("$%f_2_2$" % np.random.random())
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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())
Copy link
Member

Choose a reason for hiding this comment

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

Same question?

@tacaswell tacaswell closed this May 13, 2017
@tacaswell tacaswell reopened this May 13, 2017
@tacaswell
Copy link
Member

'power-cycled' to re-run against current master.

@@ -12,6 +12,7 @@
import pytest

import numpy as np
import matplotlib
Copy link
Member

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)
Copy link
Member

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

Copy link
Member

@tacaswell tacaswell left a 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.

Copy link
Member

@QuLogic QuLogic left a 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.

@QuLogic
Copy link
Member

QuLogic commented Aug 23, 2017

Ping? This also needs a rebase.

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Aug 29, 2017
@QuLogic QuLogic mentioned this pull request Jan 6, 2018
6 tasks
anntzer added a commit that referenced this pull request Jan 6, 2018
@QuLogic
Copy link
Member

QuLogic commented Jan 7, 2018

Thanks @tomspur; we merged a rebased version in #10180.

@QuLogic QuLogic closed this Jan 7, 2018
@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 13, 2018
@tomspur
Copy link
Contributor Author

tomspur commented Apr 2, 2018

Thanks @QuLogic for the rebase and cleanup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants