Opened 11 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 )
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)
Change History (21)
comment:1 follow-up: ↓ 2 Changed 11 years ago by
comment:2 in reply to: ↑ 1 Changed 11 years ago by
Replying to mpatel:
What if we start with mapping
sage -t ...
tosage -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 11 years ago by
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 11 years ago by
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 11 years ago by
- Description modified (diff)
comment:6 Changed 11 years ago by
- Description modified (diff)
I've opened #9225 for possible new [optional] doctesting features. Feel free to make suggestions.
comment:7 Changed 11 years ago by
- Cc leif added
comment:8 Changed 11 years ago by
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 11 years ago by
- 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 11 years ago by
- 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 11 years ago by
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: ↓ 13 Changed 11 years ago by
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 11 years ago by
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
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 81 81 Returns true if the file should not be tested 82 82 """ 83 83 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 84 87 return True 85 88 G = abspath(F) 86 89 i = G.rfind(os.path.sep) 90 # XXX The following should IMHO be performed in populatefilelist(): 91 # (Currently, populatefilelist() only looks for "__nodoctest__".) 87 92 if os.path.exists(os.path.join(G[:i], 'nodoctest.py')): 88 93 printmutex.acquire() 89 94 print "%s (skipping) -- nodoctest.py file in directory"%abs(F) 95 sys.stdout.flush() 90 96 printmutex.release() 91 97 return True 92 98 filenm = os.path.split(F)[1] … … 95 101 return True 96 102 if G.find(os.path.join('doc', 'output')) != -1: 97 103 return True 104 # XXX The following is (also/already) handled in populatefilelist(): 98 105 if not (os.path.splitext(F)[1] in ['.py', '.pyx', '.tex', '.pxi', '.sage', '.rst']): 99 106 return True 100 107 return False … … 115 122 for i in range(0,numiteration): 116 123 os.chdir(os.path.dirname(F)) 117 124 command = os.path.join(SAGE_ROOT, 'local', 'bin', 'sage-%s' % cmd) 125 # FIXME: Why call bash here? (Also, we use 'shell=True' below anyway.) 118 126 s = 'bash -c "%s %s > %s" ' % (command, filestr, outfile.name) 119 127 try: 120 128 t = time.time() … … 161 169 print sage_test_cmd(F[len(CUR)+1:]) 162 170 else: 163 171 print abs(F) 172 sys.stdout.flush() 164 173 if ol!="" and (not ol.isspace()): 165 174 if (ol[len(ol)-1]=="\n"): 166 175 ol=ol[0:len(ol)-1] 167 176 print ol 177 sys.stdout.flush() 168 178 time_dict[abs_sage_path(F)] = finished_time 169 179 if XML_RESULTS: 170 180 t = finished_time … … 192 202 """.strip() % locals()) 193 203 f.close() 194 204 print "\t [%.1f s]"%(finished_time) 205 sys.stdout.flush() 195 206 196 207 def infiles_cmp(a,b): 197 208 """ … … 231 242 base, ext = os.path.splitext(F) 232 243 if not (ext in ['.sage', '.py', '.pyx', '.tex', '.pxi', '.rst']): 233 244 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 235 253 continue 236 254 appendstr = os.path.join(root,F) 237 255 if skip(appendstr): … … 252 270 argv = [X for X in argv if X[0] != '-'] 253 271 254 272 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... 255 276 numthreads = int(argv[1]) 256 277 infiles = argv[2:] 257 278 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
- Description modified (diff)
comment:16 follow-up: ↓ 17 Changed 10 years ago by
Some random comments:
- If we keep
SAGE_TESTDIR
as a subdirectory ofDOT_SAGE
, we should call itDOT_SAGE/testing/
instead ofDOT_SAGE/tmp/
. This describes the directory better, and the directory can contain timing files, for example, which are intended to persist, not be temporary.
- Note that the script
sage-test
createsSAGE_TESTDIR
asDOT_SAGE/tmp
, and then creates a further subdirectorytmp
of this. I don't think there is any reason for this subdirectory to exist.
- Note also
sage-maketest
which writes files toSAGE_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: ↓ 18 Changed 10 years ago by
- Keywords testlong maketest parallel race condition unique doctest directories added
Replying to jhpalmieri:
- If we keep
SAGE_TESTDIR
as a subdirectory ofDOT_SAGE
, we should call itDOT_SAGE/testing/
instead ofDOT_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.
- Note that the script
sage-test
createsSAGE_TESTDIR
asDOT_SAGE/tmp
, and then creates a further subdirectorytmp
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 10 years ago by
Replying to leif:
Replying to jhpalmieri:
- If we keep
SAGE_TESTDIR
as a subdirectory ofDOT_SAGE
, we should call itDOT_SAGE/testing/
instead ofDOT_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
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
- 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.
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 ...
tosage -tp 1 ...
?