Opened 10 years ago

Closed 10 years ago

#7650 closed defect (fixed)

Fix sagenb doctesting

Reported by: mpatel Owned by: was
Priority: blocker Milestone: sage-4.3.1
Component: notebook Keywords:
Cc: was, timdumol, mhansen, jason Merged in: sage-4.3.1.rc1
Authors: Mitesh Patel Reviewers: Tim Dumol
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by mpatel)

We have not been doctesting SageNB files as often as we test Sage files.

Please apply

  • trac_7650-scripts_doctest_force_lib_v5.patch to the scripts repository. This adds two options to sage -t: -sagenb tests all SageNB files and -force_lib tests files as if they live in SAGE_ROOT/devel/sage, i.e., in the Sage library.

and replace SAGE_ROOT/makefile with

  • makefile. This makes make test and make ptest test all SageNB files, too.

Dependencies: Sage 4.3.1.alpha1, #7269.

Attachments (14)

trac_7650-sagenb_doctesting.patch (223.1 KB) - added by mpatel 10 years ago.
Add --force_lib option to sage-doctest. Use os.path.join. scripts repo.
trac_7650-scripts_doctest_force_lib.patch (8.7 KB) - added by mpatel 10 years ago.
Fix sagenb doctests. sagenb repo.
trac_7650-scripts_doctest_force_lib_v2.patch (8.7 KB) - added by mpatel 10 years ago.
Updated scripts repo patch. Replaces previous.
trac_7650-sagenb_doctesting_v2.patch (224.9 KB) - added by mpatel 10 years ago.
Quit spawned worksheet processes. Replaces previous sagenb repo patch.
trac_7650-scripts_doctest_force_lib_v3.patch (10.0 KB) - added by mpatel 10 years ago.
Auto-detect site-packages. scripts repo. Replaces previous.
trac_7650-scripts_doctest_force_lib_v4.patch (12.1 KB) - added by mpatel 10 years ago.
Adds -sagenb option. scripts repo. Replaces previous.
makefile (2.6 KB) - added by mpatel 10 years ago.
Updated SAGE_ROOT/makefile.
trac_7650-scripts_doctest_force_lib_v5.patch (16.7 KB) - added by mpatel 10 years ago.
Document new options. More os.path.join-ery. Replaces previous.
trac_7650-sagenb_doctesting_v3.patch (224.5 KB) - added by mpatel 10 years ago.
Rebased vs. #7482's v4. Replaces previous sagenb patch.
trac_7650-sagenb_doctesting_v4.patch (222.3 KB) - added by mpatel 10 years ago.
Rebased vs. 4.3.1.alpha0 + #7269. Replaces previous sagenb patch.
trac_7650-sagenb_doctesting_v5.patch (221.9 KB) - added by mpatel 10 years ago.
Fix tags / user_view asymmetry. Replaces previous sagenb patch.
trac_7650-sagenb_doctesting_v6.patch (223.0 KB) - added by mpatel 10 years ago.
Fix interact doctests for 4.3.1.alpha2 (colors.py). Replaces previous sagenb patch.
trac_7650-reviewer.patch (10.8 KB) - added by timdumol 10 years ago.
A few documentation fixes (precision errors and style), and a couple of changes to try-except blocks to make it easier to transition to Python 3
trac_7650-reviewer_v2.patch (11.5 KB) - added by mpatel 10 years ago.
Fix two doctest failures. Replaces reviewer patch. sagenb repo.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 10 years ago by mpatel

Part of the problem is that notebook directories now need to end in .sagenb.

I'm working on a patch for this and whatever other problems I can fix.

comment:2 Changed 10 years ago by mpatel

sage -t -verbose "notebook.py"                              
Traceback (most recent call last):
  File "/home/.sage//tmp/.doctest_notebook.py", line 18, in <module>
    from notebook import *
  File "/home/.sage/tmp/notebook.py", line 38, in <module>
    import css          # style
ImportError: No module named css
         [2.5 s]
exit code: 1024
 
----------------------------------------------------------------------
The following tests failed:


        sage -t -verbose "notebook.py"
Total time for all tests: 2.5 seconds

Adding

import sys
sys.path.append('/home/sage/notebook/sagenb/sagenb')
sys.path.append('/home/sage/notebook/sagenb/sagenb/notebook')

to the top of notebook.py allows testing to proceed. I'm not sure about a real solution.

Changed 10 years ago by mpatel

Add --force_lib option to sage-doctest. Use os.path.join. scripts repo.

Changed 10 years ago by mpatel

Fix sagenb doctests. sagenb repo.

comment:3 Changed 10 years ago by mpatel

  • Authors set to Mitesh Patel
  • Priority changed from major to blocker
  • Status changed from new to needs_review

Please note: The attachments (or their descriptions) are switched. I apologize for this.

Anyway, with the scripts repository patch, we can do, e.g.,

$ sage -t --force_lib sagenb/

With the sagenb repository patch applied to #7625 + #7483 + #7482 + #7514 + #7648, all tests but 3 pass. The failures are in sagenb/misc/sageinspect.py (cf. #7514). The Selenium tests still pass.

Remarks:

  • I hope I did not create false-negatives.
  • This sage-notebook thread might be relevant to some tests that evaluate cells.
  • I "untabified" cell.py, worksheet.py, notebook.py, and twist.py.
  • I added 'nodoctest' to simple/twist.py, since at least one test appears to hang indefinitely. I don't know why.
  • Feel free to lower the priority.

comment:4 Changed 10 years ago by mpatel

  • Cc mhansen added

comment:5 Changed 10 years ago by mpatel

Should we add, e.g.,

sage -t --force_lib $SAGE_ROOT/local/lib/python/site-packages/sagenb

to $SAGE_ROOT/makefile? Or sage-test?

comment:6 Changed 10 years ago by was

  • Status changed from needs_review to needs_work

I like this patch a lot. However, a nitpick: get rid of the / before tmp here:

SAGE_TESTDIR = os.path.join(DOT_SAGE, "/tmp") 

Likewise, elsewhere in the patch there are a bunch of os.path.joins combined with explicit* slashes.

Regarding adding sage -t sagenb, etc., to sage-test or whatever: "yes!"

Changed 10 years ago by mpatel

Updated scripts repo patch. Replaces previous.

Changed 10 years ago by mpatel

Quit spawned worksheet processes. Replaces previous sagenb repo patch.

comment:7 Changed 10 years ago by was

  • Milestone changed from sage-4.3.1 to sage-4.3

Changed 10 years ago by mpatel

Auto-detect site-packages. scripts repo. Replaces previous.

Changed 10 years ago by mpatel

Adds -sagenb option. scripts repo. Replaces previous.

Changed 10 years ago by mpatel

Updated SAGE_ROOT/makefile.

comment:8 Changed 10 years ago by mpatel

  • Status changed from needs_work to needs_review

With trac_7650-scripts_doctest_force_lib_v4.patch and an updated top-level makefile, we can do

  • make test
  • make ptest
  • sage -t -sagenb
  • sage -tp -sagenb

etc. Maybe we should unify the test scripts and use optparse?

comment:9 Changed 10 years ago by timdumol

  • Status changed from needs_review to needs_work

The new options to sage -t (-force_lib and -sagenb) are not documented in the help message. Please do so.

comment:10 Changed 10 years ago by was

  • Milestone changed from sage-4.3 to sage-4.3.1

I'm declaring a total feature freeze on sage-4.3.

comment:11 Changed 10 years ago by mpatel

Also: Be more agressive with os.path.join-ery.

Changed 10 years ago by mpatel

Document new options. More os.path.join-ery. Replaces previous.

comment:12 Changed 10 years ago by mpatel

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Changed 10 years ago by mpatel

Rebased vs. #7482's v4. Replaces previous sagenb patch.

comment:13 Changed 10 years ago by mpatel

  • Description modified (diff)

Changed 10 years ago by mpatel

Rebased vs. 4.3.1.alpha0 + #7269. Replaces previous sagenb patch.

comment:14 Changed 10 years ago by mpatel

  • Description modified (diff)

comment:15 Changed 10 years ago by mpatel

I've attached a rebased sagenb patch. I added """nodoctests""" to the top of sagenb.misc.ipaddr. It's also easy to make its doctest pass, but I haven't done this. Please let me know, if I should.

Changed 10 years ago by mpatel

Fix tags / user_view asymmetry. Replaces previous sagenb patch.

comment:16 Changed 10 years ago by mpatel

  • Description modified (diff)

comment:17 follow-up: Changed 10 years ago by mpatel

V5 makes it so a worksheet reconstructed from the basic representation of another has the same tags and user_view as the original.

Changed 10 years ago by mpatel

Fix interact doctests for 4.3.1.alpha2 (colors.py). Replaces previous sagenb patch.

comment:18 in reply to: ↑ 17 Changed 10 years ago by timdumol

Replying to mpatel:

V5 makes it so a worksheet reconstructed from the basic representation of another has the same tags and user_view as the original.

Shouldn't this be put in a new ticket?

comment:19 Changed 10 years ago by mpatel

Yes, but it's already here. :) In the course of fixing many failed doctests, I noticed it and a few other small problems:

  • In run_notebook.notebook_twisted: Replaced nb.directory() (not defined) with nb._dir.
  • In worksheet: Replaced self.__collaborators (AttributeError) with self.collaborators() in a few places.
  • In worksheet.set_filename:
    -        self.__dir = os.path.join(self.notebook().worksheet_directory(), filename)
    +        self.__dir = os.path.join(self.notebook()._dir, filename)
    
  • In worksheet.tags:
                 d = dict(self.__user_view)
             except AttributeError:
                 self.user_view(self.owner())
    -            d = self.__user_view
    +            d = copy.copy(self.__user_view)
             for user, val in d.iteritems():
    -            d[user] = [val]
    +            if not isinstance(val, list):
    +                d[user] = [val]
             return d
    
    This ensures the tests in Worksheet.reconstruct_from_basic pass and that calling tags does not alter the current user view, e.g., turning 1 into [1].

Also:

  • Removed argument verbose (obsolete) from Cell.set_introspect_html.

comment:20 Changed 10 years ago by timdumol

  • Cc jason added

This bags a positive review from me, except for the few changes I've made in the reviewer patch. Can someone sign them off?

Changed 10 years ago by timdumol

A few documentation fixes (precision errors and style), and a couple of changes to try-except blocks to make it easier to transition to Python 3

Changed 10 years ago by mpatel

Fix two doctest failures. Replaces reviewer patch. sagenb repo.

comment:21 Changed 10 years ago by mpatel

  • Description modified (diff)
  • Reviewers set to Tim Dumol
  • Status changed from needs_review to positive_review

V2 of the reviewer patch:

  • Adds a missing dot (.) in interact.py.
  • Moves # output depends on timezone to the input statement in worksheet.py.
  • [Pre-existing] most be defined --> must be defined.

comment:22 Changed 10 years ago by rlm

  • Merged in set to sage-4.3.1.rc1

I applied the scripts patch and replaced makefile in SAGE_ROOT. Please post a sagenb-0.5.1.spkg.

comment:23 Changed 10 years ago by rlm

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.