Opened 12 years ago

Closed 12 years ago

#10528 closed defect (fixed)

sage0.py doctest failures on sage.math

Reported by: Robert Bradshaw Owned by: Minh Van Nguyen
Priority: major Milestone: sage-4.6.1
Component: doctest coverage Keywords:
Cc: Merged in: sage-4.6.1.rc1
Authors: Robert Bradshaw Reviewers: Maarten Derickx
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

One often gets

File "/levi/scratch/robertwb/buildbot/sage-4.6/devel/sage-10515/sage/interfaces/sage0.py", line 364:
    sage: sage0.get('x')
Expected:
    "...NameError: name 'x' is not defined"
Got:
    '------------------------------------------------------------\nTraceback (most recent call last):\n  File "<ipython console>", line 1, in <module>\nNameError: name \'x\' is not defined'

Attachments (3)

10528-sage0-doctest.patch (896 bytes) - added by Robert Bradshaw 12 years ago.
10528-sage0-doctest.2.patch (739 bytes) - added by Robert Bradshaw 12 years ago.
10528-referee-fix.patch (687 bytes) - added by Robert Bradshaw 12 years ago.

Download all attachments as: .zip

Change History (19)

Changed 12 years ago by Robert Bradshaw

Attachment: 10528-sage0-doctest.patch added

comment:1 Changed 12 years ago by Robert Bradshaw

Milestone: sage-4.6.1
Status: newneeds_review

comment:2 Changed 12 years ago by Maarten Derickx

Status: needs_reviewneeds_work

Ironically it now fails on my OS X install: So it should be fixed on a higher level (i.e. in the doctest framework

sage -t "devel/sage-main/sage/interfaces/sage0.py" File "/Applications/sage-4.6.rc0/devel/sage-main/sage/interfaces/sage0.py", line 364:

sage: sage0.get('x')

Expected:

'...NameError?: name \'x\' is not defined'

Got:

"---------------------------------------------------------------------------\nNameError Traceback (most recent call last)\n\n/Applications/sage-4.6.rc0/data/extcode/sage/<ipython console> in <module>()\n\nNameError: name 'x' is not defined"

1 items had failures:

1 of 6 in main.example_13

*Test Failed* 1 failures. For whitespace errors, see the file /Users/maarten/.sagetmp/.doctest_sage0.py

[21.7 s]


The following tests failed:

sage -t "devel/sage-main/sage/interfaces/sage0.py"

Total time for all tests: 21.8 seconds

comment:3 Changed 12 years ago by Maarten Derickx

There also seems to be an error in the fixdoctest option, since -fixdoctests doesn't fix the doctest for me. (i.e. it outputs exactly the same file)

maarten-derickxs-macbook-pro:sage-main maarten$ sage -fixdoctests sage/interfaces/sage0.py 
maarten-derickxs-macbook-pro:sage-main maarten$ diff sage/interfaces/sage0.py sage/interfaces/sage0.py.out
maarten-derickxs-macbook-pro:sage-main maarten

Changed 12 years ago by Robert Bradshaw

Attachment: 10528-sage0-doctest.2.patch added

comment:4 Changed 12 years ago by Robert Bradshaw

Status: needs_workneeds_review

I agree something is up with the doctest framework, but I think this is worth getting in for the next release, so here's a simpler fix.

comment:5 Changed 12 years ago by Maarten Derickx

Ok the test passes now. But I'm not sure if it should pass. I looked more into it. And I found out that the communication meganism used to send strings between the second sage instance sage0 and the sage main sage install actually sends different strings on different machines. So I guess the doctest fails cause of a real bug in the sage0 interface.

On what kind of machine is patchbot running?

comment:6 in reply to:  5 Changed 12 years ago by Robert Bradshaw

Replying to mderickx:

Ok the test passes now. But I'm not sure if it should pass. I looked more into it. And I found out that the communication meganism used to send strings between the second sage instance sage0 and the sage main sage install actually sends different strings on different machines. So I guess the doctest fails cause of a real bug in the sage0 interface.

The platform dependence is very strange, but given that it's just testing that the clear() method does what it should I think it's a fine doctest.

On what kind of machine is patchbot running?

sage.math

comment:7 Changed 12 years ago by Maarten Derickx

The strange thing is that I don't seem to be able to reproduce the bug on sage.math

mderickx@sage:/levi/scratch/robertwb/buildbot/sage-4.6$ ./sage -t devel/sage-10515/sage/interfaces/sage0.py 
sage -t  "devel/sage-10515/sage/interfaces/sage0.py"        
	 [9.8 s]
 
----------------------------------------------------------------------
All tests passed!
Total time for all tests: 9.8 seconds

comment:8 Changed 12 years ago by Robert Bradshaw

I am as mystified as you:

robertwb@sage:/levi/scratch/robertwb/buildbot/sage-4.6$ ./sage -t devel/sage-main/sage/interfaces/sage0.py 
sage -t  "devel/sage-main/sage/interfaces/sage0.py"         
**********************************************************************
File "/levi/scratch/robertwb/buildbot/sage-4.6/devel/sage-main/sage/interfaces/sage0.py", line 364:
    sage: sage0.get('x')
Expected:
    "...NameError: name 'x' is not defined"
Got:
    '------------------------------------------------------------\nTraceback (most recent call last):\n  File "<ipython console>", line 1, in <module>\nNameError: name \'x\' is not defined'
**********************************************************************
1 items had failures:
   1 of   6 in __main__.example_13
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/robertwb/.sage//tmp/.doctest_sage0.py
	 [9.2 s]
 
----------------------------------------------------------------------
The following tests failed:


	sage -t  "devel/sage-main/sage/interfaces/sage0.py"
Total time for all tests: 9.2 seconds

I've also seen this on my OS X box from time to time.

I view "something strange is up with the doctexting framework" as a different bug than "the sage0 doctests are flakey," and the latter has a fix now while the former is who knows how long out. I don't consider it covering the bug to re-factor the doctest to be less rigid but still test what's being tested for.

comment:9 Changed 12 years ago by Maarten Derickx

Status: needs_reviewpositive_review

After giving it some thought I agree that the doctest is really still is testing what it should test after the patch so I give it a positive review and created http://trac.sagemath.org/sage_trac/ticket/10539 for the other problem.

comment:10 Changed 12 years ago by Jeroen Demeyer

Authors: Robert Bradshaw
Milestone: sage-4.6.1sage-4.6.2
Reviewers: Maarten Derickx
Status: positive_reviewneeds_work

I would prefer some explanation and a reference to the ticket in the patched example (hence, needs_work).

Changed 12 years ago by Robert Bradshaw

Attachment: 10528-referee-fix.patch added

comment:11 Changed 12 years ago by Robert Bradshaw

Milestone: sage-4.6.2sage-4.6.1
Status: needs_workpositive_review

I don't think such an explanation is strictly necessary, but here's a patch adding it if you want it, and I'm re-instating the initial positive review. Also I think this patch would be extremely valuable to get into the Sage release as it will make the patchbot much more useful (i.e. we'll actually have tickets with all tests passing).

comment:12 in reply to:  11 ; Changed 12 years ago by Jeroen Demeyer

Replying to robertwb:

Also I think this patch would be extremely valuable to get into the Sage release as it will make the patchbot much more useful (i.e. we'll actually have tickets with all tests passing).

Actually, I prefer not adding new patches now unless there is a very good reason for it. For the patchbot, you could simply use a custom version of Sage with this ticket included (I can even make that special patchbot release if you want).

comment:13 in reply to:  12 Changed 12 years ago by Robert Bradshaw

Replying to jdemeyer:

Replying to robertwb:

Also I think this patch would be extremely valuable to get into the Sage release as it will make the patchbot much more useful (i.e. we'll actually have tickets with all tests passing).

Actually, I prefer not adding new patches now unless there is a very good reason for it.

Sure, that's why I was trying to make a good argument for it. It's 100% safe.

For the patchbot, you could simply use a custom version of Sage with this ticket included (I can even make that special patchbot release if you want).

I could patch my bot, but ideally many people will be running patchbots on several different machines, and it'd be nice if people could just run the latest release, rather than the latest release + this patch.

comment:14 Changed 12 years ago by Jeroen Demeyer

Allright, you convinced me.

comment:15 Changed 12 years ago by Robert Bradshaw

Thanks.

comment:16 Changed 12 years ago by Jeroen Demeyer

Merged in: sage-4.6.1.rc1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.