Opened 6 years ago
Closed 6 years ago
#17718 closed enhancement (fixed)
Further cleanup of expect.py
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage6.6 
Component:  interfaces  Keywords:  
Cc:  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Vincent Delecroix, Marc Mezzarobba, Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  c8d1a28 (Commits, GitHub, GitLab)  Commit:  c8d1a289e95184f3f6f5528e8fb8ee4638ab47c0 
Dependencies:  #17704  Stopgaps: 
Description
Change History (21)
comment:1 Changed 6 years ago by
 Branch set to u/jdemeyer/ticket/17718
 Created changed from 02/03/15 09:33:17 to 02/03/15 09:33:17
 Modified changed from 02/03/15 09:33:17 to 02/03/15 09:33:17
comment:2 Changed 6 years ago by
 Commit set to d556229dd9fec47e492a3543b0815ce02b4870de
 Status changed from new to needs_review
comment:3 followup: ↓ 4 Changed 6 years ago by
Why
if self.silent:  return + return self.interface
If this is necessary, could you add a doctest?
comment:4 in reply to: ↑ 3 Changed 6 years ago by
Replying to vdelecroix:
Why
if self.silent:  return + return self.interfaceIf this is necessary, could you add a doctest?
It's to allow with StdOutContext(...) as ...
as opposed to just with StdOutContext(...)
.
So it's covered by this doctest change:
sage: with StdOutContext(gp): ... gp('1+1') ... +sage: with StdOutContext(Gp()) as g: +....: g('1+1')
comment:5 Changed 6 years ago by
 Branch changed from u/jdemeyer/ticket/17718 to u/mmezzarobba/17718expect
 Commit changed from d556229dd9fec47e492a3543b0815ce02b4870de to 568b6a8e4478fe93f70841efc664a9992e820386
comment:6 followup: ↓ 7 Changed 6 years ago by
 Status changed from needs_review to needs_work
Why that extra commit?
comment:7 in reply to: ↑ 6 ; followups: ↓ 8 ↓ 9 Changed 6 years ago by
Replying to jdemeyer:
Why that extra commit?
As the commit message says: to make the optional test involving octave pass also when the tests are run with optional=octave
, not optional=sage,octave
.
comment:8 in reply to: ↑ 7 ; followup: ↓ 10 Changed 6 years ago by
Replying to mmezzarobba:
Replying to jdemeyer:
Why that extra commit?
As the commit message says: to make the optional test involving octave pass also when the tests are run with
optional=octave
, notoptional=sage,octave
.
I'm not aware of any general rule that tests should pass when run with ./sage t optional=octave
. I prefer to have as few tests as possible optional.
comment:9 in reply to: ↑ 7 Changed 6 years ago by
 Branch changed from u/mmezzarobba/17718expect to u/jdemeyer/ticket/17718
 Commit changed from 568b6a8e4478fe93f70841efc664a9992e820386 to d556229dd9fec47e492a3543b0815ce02b4870de
 Status changed from needs_work to needs_review
In any case, this could be discussed in a different ticket.
comment:10 in reply to: ↑ 8 Changed 6 years ago by
 Reviewers set to Vincent Delecroix, Marc Mezzarobba
 Status changed from needs_review to positive_review
Replying to jdemeyer:
I'm not aware of any general rule that tests should pass when run with
./sage t optional=octave
.
What is the point of having an option optional=octave
that does not imply optional=sage
, then?
I prefer to have as few tests as possible optional.
I agree in principle, but here I don't think the example without its optional
part is a meaningful test case.
Anyway, I'm fine with leaving that change out if you prefer.
comment:11 followup: ↓ 13 Changed 6 years ago by
 Status changed from positive_review to needs_work
This leads to random failures in the R interface on my desktop (HaswellE Linux x86_64, background computation running)
sage t long src/sage/interfaces/r.py ********************************************************************** File "src/sage/interfaces/r.py", line 24, in sage.interfaces.r Failed example: 1/x Expected: [1] 0.09615385 0.17857143 0.32258065 0.15625000 0.04608295 Got: <BLANKLINE> command timed out: 1200 seconds without output running ['./sage', 't', 'p', '8', 'all', 'long', 'logfile=logs/ptestlong.log'], attempting to kill process killed by signal 9 program finished with exit code 1 elapsedTime=2461.278640
comment:12 Changed 6 years ago by
another failed example (seems to be falling over a prompt in the R docs if you are unlucky with the timing):
$ SAGE_PEXPECT_LOG=yes sage bt long verbose src/sage/interfaces/r.py [...] Trying (line 1955): print length._sage_doc_() Expecting: length package:base R Documentation ... <BLANKLINE> ok [0.04 s] Trying (line 1959): sig_on_count() Expecting: 0 ok [0.00 s] Trying (line 1969): length = r.length Expecting nothing ok [0.00 s] Trying (line 1970): print length._sage_src_() Expecting: Trying (line 1970): print length._sage_src_() Expecting: function (x) .Primitive("length") ok [0.00 s] Trying (line 1973): sig_on_count() Expecting: 0 ok [0.00 s] Trying (line 1981): length = r.length Expecting nothing ok [0.00 s] Trying (line 1982): length([1,2,3]) Expecting: [1] 3 ... hangs ...
pexpect log:
[...] Examples: length(diag(4)) # = 16 (4 x 4) length(options()) # 12 or more length(y ~ x1 + x2 + x3) # 3 length(expression(x, {y < x^2; y+2}, x^y)) # 3 ## from example(warpbreaks) require(stats) fm1 < lm(breaks ~ wool * tension, data = warpbreaks) length(fm1$call) # 3, lm() and two arguments. length(formula(fm1)) # 3, ~ lhs rhs __SAGE__R__PROMPT__> 1+310416896; [1] 310416897 __SAGE__R__PROMPT__> length length function (x) .Primitive("length") __SAGE__R__PROMPT__> 1+1371999567; [1] 1.372e+09 __SAGE__R__PROMPT__> quit(save="no") __SAGE__R__PROMPT__> __SAGE__R__PROMPT__> ^C quit(save="no") sage2 < 1 sage2 < 1 1+1305756531; quit(save="no")
comment:13 in reply to: ↑ 11 Changed 6 years ago by
Replying to vbraun:
This leads to random failures in the R interface on my desktop
Are you really sure that this is caused by this ticket? The point is that this ticket is really a cleanup ticket, with almost no functional changes.
comment:14 Changed 6 years ago by
Well maybe it triggers a preexisting bug. All I know for sure is that doctests get stuck in R on my machine with it.
comment:15 Changed 6 years ago by
How common is this failure?
comment:16 Changed 6 years ago by
Maybe 50% when the system is under load...
comment:17 Changed 6 years ago by
When I have some time, I'll try to reproduce it. The only possible reason that I can think of is the change self._interrupt()
> self.interrupt()
If that's the cause, I suggest to just revert that.
comment:18 Changed 6 years ago by
 Commit changed from d556229dd9fec47e492a3543b0815ce02b4870de to c8d1a289e95184f3f6f5528e8fb8ee4638ab47c0
Branch pushed to git repo; I updated commit sha1. New commits:
c8d1a28  Revert interrupt change

comment:19 Changed 6 years ago by
 Status changed from needs_work to needs_review
I confirm the issue and this should fix it.
comment:20 Changed 6 years ago by
 Milestone changed from sage6.5 to sage6.6
 Reviewers changed from Vincent Delecroix, Marc Mezzarobba to Vincent Delecroix, Marc Mezzarobba, Ralf Stephan
 Status changed from needs_review to positive_review
Patchbot says fine. Tests in interfaces
on my machine pass too. As the code was already reviewed by others I dare to set positive.
comment:21 Changed 6 years ago by
 Branch changed from u/jdemeyer/ticket/17718 to c8d1a289e95184f3f6f5528e8fb8ee4638ab47c0
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Clean up in expect.py
Further cleanup of interfaces