Opened 9 years ago

Closed 7 years ago

#10655 closed defect (fixed)

Fix optional animate.py doctests

Reported by: kcrisman Owned by: mvngu
Priority: minor Milestone: sage-5.1
Component: doctest coverage Keywords: convert ImageMagick animate optional sd31 sd41
Cc: Merged in: sage-5.1.beta6
Authors: John Palmieri Reviewers: John Palmieri, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jhpalmieri)

Pretty much all the optional tests in animate.py are fine to do if you possess ImageMagick? and the convert command. Some of them should really then save to a temp file (in the doctest).

This ticket is to make sure that all of them have the actual keyword convert so that optional doctesting works, and to fix those temp files. See #7981 for background.

Apply trac_10655-optional-doctest.patch.

Attachments (1)

trac_10655-optional-doctest.patch (865 bytes) - added by jhpalmieri 7 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by jhpalmieri

The patch at #11170 fixes this (although it uses the keyword ImageMagick instead of convert).

comment:2 Changed 8 years ago by jhpalmieri

  • Keywords sd31 added

I think this can be closed now, since #11170 has been merged. Can anyone confirm this?

comment:3 Changed 8 years ago by kcrisman

  • Reviewers set to John Palmieri, Karl-Dieter Crisman

Overall it's nearly ready to close. I think you missed one of the tests. See this spot in animate.py - I think that line 198 needs the keyword too.

Yup.

File "/Users/.../sage-5.0/devel/sage-main/sage/plot/animate.py", line 198:
    sage: b.show() # optional
Exception raised:
    Error: Neither ImageMagick nor ffmpeg appears to be installed. Saving an
    animation to a GIF file or displaying an animation requires one of these
    packages, so please install one of them and try again.

    See www.imagemagick.org and www.ffmpeg.org for more information.
**********************************************************************

If I remembered how to test everything optional except a certain keyword, it wouldn't have taken so much hunting to find that snippet. Didn't you implement that somewhere? But I couldn't find it in the developer guide for 5.0, so it must not have gotten in yet.

comment:4 Changed 7 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Description modified (diff)
  • Status changed from new to needs_review

Here's a patch adding optional -- ImageMagick marks to three doctests. If you run something like sage -t --only-optional=ImageMagick, then it will run tests marked with ImageMagick, but I don't think we've implemented anything which says to skip tests marked a certain way.

comment:5 Changed 7 years ago by jhpalmieri

  • Description modified (diff)

comment:6 Changed 7 years ago by kcrisman

  • Status changed from needs_review to needs_work

Umm, the second two doctests might be optional for some reason, but they don't require ImageMagick. I just tested this by renaming convert so that

sage -t --only-optional=ImageMagick

failed, and those tests did not shows up as ones that failed. If you look at the code, too, it doesn't use anything except the initialization (which doesn't need convert) and graphics_array, which clearly doesn't either.

I don't know what you want to do with those, but at any rate this isn't the right solution. Maybe instead put a little note that these don't require convert or something?

Also, please use a more descriptive commit message.

comment:7 follow-up: Changed 7 years ago by jhpalmieri

Hmm. Why are they optional in the first place, then? I'll remove those tags, anyway.

Changed 7 years ago by jhpalmieri

comment:8 in reply to: ↑ 7 Changed 7 years ago by kcrisman

  • Status changed from needs_work to positive_review

Hmm. Why are they optional in the first place, then? I'll remove those tags, anyway.

My thoughts exactly.

Anyway, this looks good. Testing with --only-optional... testing with --optional... good to go.

comment:9 Changed 7 years ago by jdemeyer

  • Milestone set to sage-5.1

comment:10 Changed 7 years ago by jhpalmieri

  • Keywords sd41 added

comment:11 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.1.beta6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.