Opened 10 years ago

Closed 8 years ago

#9224 closed enhancement (duplicate)

Unify sage-test and sage-ptest

Reported by: mpatel Owned by: mvngu
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: doctest coverage Keywords: testlong maketest parallel race condition unique doctest directories
Cc: cwitty, ddrake, drkirkby, jhpalmieri, leif, wjp Merged in:
Authors: Reviewers: David Roe
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by leif)

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.

Attachments (1)

doctest.py (2.5 KB) - added by mpatel 10 years ago.
Doctester class pseudo-interface. Not a patch.

Download all attachments as: .zip

Change History (21)

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

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 10 years ago by ddrake

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 10 years ago by ddrake

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"):
            ol=ol[0:len(ol)-1]
        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 10 years ago by ddrake

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 10 years ago by mpatel

  • Description modified (diff)

comment:6 Changed 10 years ago by mpatel

  • Description modified (diff)

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

comment:7 Changed 10 years ago by leif

  • Cc leif added

Changed 10 years ago by mpatel

Doctester class pseudo-interface. Not a patch.

comment:8 Changed 10 years ago by mpatel

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.

Thoughts?

comment:9 Changed 10 years ago by mpatel

  • Description modified (diff)

If the interface above (or a variation) is reasonable, we could populate doctest.py with most of the non-parsing-related, non-redundant parts of sage-doctest and sage-(p)test. To ensure that we test doctest.py 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 10 years ago by mpatel

  • Cc cwitty drkirkby 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 10 years ago by cwitty

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 follow-up: Changed 10 years ago by mhansen

Quite awhile ago, I had some success with using nose ( http://code.google.com/p/python-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 10 years ago by drkirkby

Replying to mhansen:

Quite awhile ago, I had some success with using nose ( http://code.google.com/p/python-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.

Dave

comment:14 Changed 10 years ago by leif

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], 'nodoctest.py')):
    8893        printmutex.acquire()
    8994        print "%s (skipping) -- nodoctest.py 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, outfile.name)
    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()
    195206
    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 "nodoctest.py"
     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                    # "nodoctest.py" 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] != '-']
    253271
    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 10 years ago by leif

  • Description modified (diff)

comment:16 follow-up: Changed 9 years ago by jhpalmieri

Some random comments:

  1. If we keep SAGE_TESTDIR as a subdirectory of DOT_SAGE, we should call it DOT_SAGE/testing/ instead of DOT_SAGE/tmp/. This describes the directory better, and the directory can contain timing files, for example, which are intended to persist, not be temporary.
  1. Note that the script sage-test creates SAGE_TESTDIR as DOT_SAGE/tmp, and then creates a further subdirectory tmp of this. I don't think there is any reason for this subdirectory to exist.
  1. Note also sage-maketest which writes files to SAGE_TESTDIR. (It also has a different default setting for this variable.) This script may not require any modification, but it's another one to keep an eye on.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 9 years ago by leif

  • Keywords testlong maketest parallel race condition unique doctest directories added

Replying to jhpalmieri:

  1. If we keep SAGE_TESTDIR as a subdirectory of DOT_SAGE, we should call it DOT_SAGE/testing/ instead of DOT_SAGE/tmp/. This describes the directory better, and the directory can contain timing files, for example, which are intended to persist, not be temporary.

At least for the temporary files, I think the name of the subdirectory should also contain the hostname and the PID of the parent process (just like sage-ptest now manages this, cf. #9739), to support e.g. running make testlong simultaneously from different machines sharing the same filesystem, i.e., sharing the DOT_SAGE directory or whatever SAGE_TESTDIR might be, and also running different instances of e.g. make testlong or sage -t ... on the same machine at the same time.

(At least Dave requested this feature, cf. his comment here and here, and my suggestion.)

Timings should perhaps have their own, persistent directory. Either we accept the possibility of race conditions there, or we should add e.g. the hostname and the time (the former reasonable anyway), eventually also the PID, to the filenames.


  1. Note that the script sage-test creates SAGE_TESTDIR as DOT_SAGE/tmp, and then creates a further subdirectory tmp of this. I don't think there is any reason for this subdirectory to exist.

Yep, that's odd. We have a whole bunch of tmp / temp subdirectories all around.

comment:18 in reply to: ↑ 17 Changed 9 years ago by jhpalmieri

Replying to leif:

Replying to jhpalmieri:

  1. If we keep SAGE_TESTDIR as a subdirectory of DOT_SAGE, we should call it DOT_SAGE/testing/ instead of DOT_SAGE/tmp/. This describes the directory better, and the directory can contain timing files, for example, which are intended to persist, not be temporary.

At least for the temporary files, I think the name of the subdirectory should also contain the hostname and the PID of the parent process.

Right, my intention was, by default anyway, to create such a temporary directory inside SAGE_TESTDIR. Or we can have separate environment variables for the (parent of the) testing directory and the directory containing timing files, etc.

comment:19 Changed 9 years ago by roed

Note that Robert Bradshaw has a bunch of code that I'm working on incorporating into Sage that redoes the doctesting framework (and adds various nice features). See #12415.

comment:20 Changed 8 years ago by jdemeyer

  • Milestone set to sage-duplicate/invalid/wontfix
  • Resolution set to duplicate
  • Reviewers set to David Roe
  • Status changed from new to closed

Superseded by #12415.

Note: See TracTickets for help on using tickets.