Opened 7 years ago

Closed 5 years ago

#13297 closed enhancement (fixed)

Fix broken rst2sws and add doctests for the scripts rst2txt and rst2sws

Reported by: slabbe Owned by: slabbe
Priority: major Milestone: sage-6.4
Component: doctest coverage Keywords:
Cc: Merged in:
Authors: Sébastien Labbé Reviewers: Clemens Heuberger
Report Upstream: N/A Work issues:
Branch: 8e9cbb3 (Commits) Commit: 8e9cbb354eb3ec727628250b1d378bddc0e78f0e
Dependencies: Stopgaps:

Description (last modified by cheuberg)

sage -rst2sws and sage -rst2txt introduced in #11459 are not doctested and are suceptible to be broken in the future. Such doctests can be added to the file sage/tests/cmdline.py.

Indeed, sage -rst2sws was broken, cf. #17371. This is fixed here and doctests are now added.

Change History (16)

comment:1 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:2 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:4 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:5 Changed 5 years ago by slabbe

  • Branch set to u/slabbe/13297
  • Commit set to 242f03b0a55224e8b0ca3a3fa9f6f9173907fd20
  • Status changed from new to needs_review

Motivated by the (second) issue found in #17371, I finally wrote doctests for cmdline rst2txt and rst2sws.

Needs review!

New commits:

​242f03bTrac #13297: Basic cmd line tests for rst2txt and rst2sws

comment:6 Changed 5 years ago by kcrisman

Ah! Maybe we can also use this to test other scripts of this nature... see #17377.

Just a comment (I don't have time immediately to try this): What of sage --rst2txt - is that eventually a new way? I think that is now supported, anyway, maybe should put that instead for consistency's sake (not that I care, I know others do).

comment:7 Changed 5 years ago by cheuberg

  • Authors set to Sébastien Labbé

comment:8 Changed 5 years ago by cheuberg

Would it be feasible to check that the output of -rst2sws does what it should, e.g. using the python tarfile module to extract sage_worksheet/worksheet.html and check its content?

comment:9 Changed 5 years ago by kcrisman

I think that should be pretty reasonable, as long as one uses temp files throughout.

comment:10 Changed 5 years ago by git

  • Commit changed from 242f03b0a55224e8b0ca3a3fa9f6f9173907fd20 to 8e9cbb354eb3ec727628250b1d378bddc0e78f0e

Branch pushed to git repo; I updated commit sha1. New commits:

​8e9cbb3Trac #13297: Adding and fixing tests for rst2txt and rst2sws

comment:11 Changed 5 years ago by slabbe

Thanks for your comments, I fix them accordingly. I also added more tests.

comment:12 Changed 5 years ago by cheuberg

  • Description modified (diff)
  • Summary changed from Add doctests for the scripts rst2txt and rst2sws to Fix broken rst2sws and add doctests for the scripts rst2txt and rst2sws

comment:13 Changed 5 years ago by cheuberg

  • Reviewers set to Clemens Heuberger

The new tests seem to be reasonable. Doctests pass. The .sws file generated in the test can be imported into the sage worksheet, so it seems to be ok. I manually checked ./sage -docbuild file=src/sage/tests/cmdline.py html to check that no new ReSt errors are introduced by this ticket (the list at the top is rendered messily, but that's not this ticket's fault).

For me, this is a positive review. I do not yet press the button in order to give Karl-Dieter or others still a possibility to comment.

comment:14 Changed 5 years ago by kcrisman

Nono, looks fine to me too but I don't have time to test and think about it, so feel free!

comment:15 Changed 5 years ago by cheuberg

  • Status changed from needs_review to positive_review

comment:16 Changed 5 years ago by vbraun

  • Branch changed from u/slabbe/13297 to 8e9cbb354eb3ec727628250b1d378bddc0e78f0e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.