Opened 12 years ago

Last modified 10 years ago

#9224 closed enhancement

Unify sage-test and sage-ptest — at Version 15

Reported by: Mitesh Patel Owned by: Minh Van Nguyen
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: doctest coverage Keywords: testlong maketest parallel race condition unique doctest directories
Cc: Carl Witty, Dan Drake, David Kirkby, John Palmieri, Leif Leonhardy, wjp Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Leif Leonhardy)

We currently have separate single and multi-threaded Sage doctest scripts. In particular, sage -t ... invokes SAGE_ROOT/local/bin/sage-test and sage -tp ... invokes sage-ptest. These files share many lines of almost functionally identical identical code. Unifying these scripts should make it easier to maintain and extend the Sage doctest system.

Related: #2379, #7993, #7995, #8641, #9225, #9243, #9316, #9739.

Change History (16)

comment:1 Changed 12 years ago by Mitesh Patel

It seems the merger is overdue, although I doubt I can work on it in the immediate future, e.g., before #8641 closes.

What if we start with mapping sage -t ... to sage -tp 1 ...?

comment:2 in reply to:  1 Changed 12 years ago by Dan Drake

Replying to mpatel:

What if we start with mapping sage -t ... to sage -tp 1 ...?

This sounds great to me. It immediately removes the code duplication and is very easy to implement. Perhaps it's worth asking on sage-devel about this, in case anyone knows of a substantive difference between "-t" and "-tp 1", but I think this is a good idea.

comment:3 Changed 12 years ago by Dan Drake

I'm looking at this, and it might be a bit trickier than I thought. Doing "sage -tp 1 ..." tests to see if Sage starts, which we don't want. That's easy to fix, though. What might be harder is that sage-ptest has some awful code in it. The test_file function, according to comments, is supposed to return a 4-tuple, but sometimes it returns a 3-tuple (line 122); there's a bare "except", which is probably not a good idea; the function that processes the tuples just mentioned does this:

  F = result[0]
  ret = result[1]
  finished_time = result[2]
  ol = result[3]

The "result" usually is a 4-tuple: (filename, return_code, time, some_string). But sometimes it returns a 3-tuple: (-5, 0, some_string). So the code above sometimes gets a filename for F, and sometimes gets the integer -5. Then there's this:

  if ol!="" and (not ol.isspace()):
        if (ol[len(ol)-1]=="\n"):
        print ol

Why is that not

  if ol and not ol.isspace():
    print ol.rstrip()

? There is some strange stuff in sage-ptest.

comment:4 Changed 12 years ago by Dan Drake

See #9243 for a related ticket -- it feels weird to be or'ing return codes together when the basic return codes are not powers of 2.

comment:5 Changed 12 years ago by Mitesh Patel

Description: modified (diff)

comment:6 Changed 12 years ago by Mitesh Patel

Description: modified (diff)

I've opened #9225 for possible new [optional] doctesting features. Feel free to make suggestions.

comment:7 Changed 12 years ago by Leif Leonhardy

Cc: Leif Leonhardy added

Changed 12 years ago by Mitesh Patel

Attachment: added

Doctester class pseudo-interface. Not a patch.

comment:8 Changed 12 years ago by Mitesh Patel

I've attached a first take on an interface for a Sage Doctester class. Please see this comment at #9225 for possible uses. I'm not sure whether to put this in the Sage library (under misc/) or in SAGE_LOCAL/bin. But I think we'll need to add sage-doctest to the unification efforts.


comment:9 Changed 12 years ago by Mitesh Patel

Description: modified (diff)

If the interface above (or a variation) is reasonable, we could populate with most of the non-parsing-related, non-redundant parts of sage-doctest and sage-(p)test. To ensure that we test regularly, I suggest we put it in the sage repository, under misc/. Then we'd put just the command-line parsing code in a sage-test in the scripts repository. This script would import sage.misc.doctest and instantiate/call a Doctester object to run the tests.

More keyword arguments for Doctester's constructor: timeout, timeout_long, testdir, and various Valgrind settings.

comment:10 Changed 12 years ago by Mitesh Patel

Cc: Carl Witty David Kirkby added

Just requesting feedback on re-{design,writ,refactor}-ing the doctesting system --- but only when it's convenient, as I can't work on this immediately.

comment:11 Changed 12 years ago by Carl Witty

I do like the idea of merging sage-test and sage-ptest.

Your Doctester API seems reasonable at first glance; there might have to be minor changes when it's actually implemented, but it's surely basically sound. (I mean that it has to be possible to split sage-(p)test into pieces, one of which implements your Doctester API, and one which calls it.)

Merging sage-doctest into the same code seems trickier, especially if you mean it to also run in the same process.

I'm afraid I'm not giving very good feedback... "sounds like a good idea, but it might be hard" :) The problem is that at the moment I don't understand sage-test, sage-ptest, and sage-doctest well enough to give more detailed comments.

comment:12 Changed 12 years ago by Mike Hansen

Quite awhile ago, I had some success with using nose ( ) to test Sage. I'm not sure if I still have that code. It might be something to look into since a number of projects use it for testing.

comment:13 in reply to:  12 Changed 12 years ago by David Kirkby

Replying to mhansen:

Quite awhile ago, I had some success with using nose ( ) to test Sage. I'm not sure if I still have that code. It might be something to look into since a number of projects use it for testing.

Sqlalchemy (a standard package in Sage) has a set of self-tests which rely on 'nose'. I did ask on sage-devel a few weeks back if that should be included in Sage, but what response I did get was negative.

I'm keen we have more testing of the individual components of Sage. At the minute, few have spkg-check files, and as I say, at least sqlalchemy will not work without nose.

#9281 has a list of all the standard packages and whether they have an spkg-check file. The number is depressing small - about 20% of them.


comment:14 Changed 12 years ago by Leif Leonhardy

I have some more comments (here just on sage-ptest) I'll post inline also here:

  • sage-ptest

    diff --git a/sage-ptest b/sage-ptest
    a b  
    8181    Returns true if the file should not be tested
    8282    """
    8383    if not os.path.exists(F):
     84        # XXX IMHO this should never happen; in case it does, it's certainly
     85        #     an error to be reported (either filesystem, or bad name specified
     86        #     on the command line). -leif
    8487        return True
    8588    G = abspath(F)
    8689    i = G.rfind(os.path.sep)
     90    # XXX The following should IMHO be performed in populatefilelist():
     91    #     (Currently, populatefilelist() only looks for "__nodoctest__".)
    8792    if os.path.exists(os.path.join(G[:i], '')):
    8893        printmutex.acquire()
    8994        print "%s (skipping) -- file in directory"%abs(F)
     95        sys.stdout.flush()
    9096        printmutex.release()
    9197        return True
    9298    filenm = os.path.split(F)[1]
    95101        return True
    96102    if G.find(os.path.join('doc', 'output')) != -1:
    97103        return True
     104    # XXX The following is (also/already) handled in populatefilelist():
    98105    if not (os.path.splitext(F)[1] in ['.py', '.pyx', '.tex', '.pxi', '.sage', '.rst']):
    99106        return True
    100107    return False
    115122    for i in range(0,numiteration):
    116123        os.chdir(os.path.dirname(F))
    117124        command = os.path.join(SAGE_ROOT, 'local', 'bin', 'sage-%s' % cmd)
     125        # FIXME: Why call bash here? (Also, we use 'shell=True' below anyway.)
    118126        s = 'bash -c "%s %s > %s" ' % (command, filestr,
    119127        try:
    120128            t = time.time()
    161169        print sage_test_cmd(F[len(CUR)+1:])
    162170    else:
    163171        print abs(F)
     172    sys.stdout.flush()
    164173    if ol!="" and (not ol.isspace()):
    165174        if (ol[len(ol)-1]=="\n"):
    166175            ol=ol[0:len(ol)-1]
    167176        print ol
     177        sys.stdout.flush()
    168178    time_dict[abs_sage_path(F)] = finished_time
    169179    if XML_RESULTS:
    170180        t = finished_time
    192202        """.strip() % locals())
    193203        f.close()
    194204    print "\t [%.1f s]"%(finished_time)
     205    sys.stdout.flush()
    196207def infiles_cmp(a,b):
    197208    """
    231242                base, ext = os.path.splitext(F)
    232243                if not (ext in ['.sage', '.py', '.pyx', '.tex', '.pxi', '.rst']):
    233244                    continue
    234                 elif '__nodoctest__' in files:
     245                elif '__nodoctest__' in files: # XXX Shouldn't this be 'lfiles'?
     246                    # Also, this test should IMHO be in the outer loop (1 level).
     247                    # Furthermore, the current practice is to put ""
     248                    # files in the directories that should be skipped, not
     249                    # "__nodoctest__". (I haven't found a single instance of the
     250                    # latter in Sage 4.6.1.alpha3.)
     251                    # "" is handled in skip() (!), to also be fixed.
     252                    # -leif
    235253                    continue
    236254                appendstr = os.path.join(root,F)
    237255                if skip(appendstr):
    252270    argv = [X for X in argv if X[0] != '-']
    254272    try:
     273        # FIXME: Nice, but <NUMTHREADS> should immediately follow '-tp' etc.,
     274        #        i.e., be the next argument. We might have file or directory
     275        #        names that properly convert to an int...
    255276        numthreads = int(argv[1])
    256277        infiles = argv[2:]
    257278    except ValueError: # can't convert first arg to an integer: arg was probably omitted

(This is against Sage 4.6.1.alpha3.)

The comments all refer to inconsistencies; the only actual change is flushing the output since it currently comes in bursts, which is IMHO odd for watching it. I don't think this measurably slows down doctesting...

comment:15 Changed 12 years ago by Leif Leonhardy

Description: modified (diff)
Note: See TracTickets for help on using tickets.