Opened 6 years ago

Closed 6 years ago

#17718 closed enhancement (fixed)

Further clean-up of expect.py

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.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:

Status badges

Description


Change History (21)

comment:1 Changed 6 years ago by jdemeyer

  • 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 jdemeyer

  • Commit set to d556229dd9fec47e492a3543b0815ce02b4870de
  • Status changed from new to needs_review

New commits:

8501bb1Clean up in expect.py
d556229Further clean-up of interfaces

comment:3 follow-up: Changed 6 years ago by vdelecroix

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 jdemeyer

Replying to vdelecroix:

Why

 if self.silent:
-     return
+     return self.interface

If 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 mmezzarobba

  • Branch changed from u/jdemeyer/ticket/17718 to u/mmezzarobba/17718-expect
  • Commit changed from d556229dd9fec47e492a3543b0815ce02b4870de to 568b6a8e4478fe93f70841efc664a9992e820386

comment:6 follow-up: Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Why that extra commit?

comment:7 in reply to: ↑ 6 ; follow-ups: Changed 6 years ago by 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, not --optional=sage,octave.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by jdemeyer

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, not --optional=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 jdemeyer

  • Branch changed from u/mmezzarobba/17718-expect 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 mmezzarobba

  • 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 follow-up: Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

This leads to random failures in the R interface on my desktop (Haswell-E 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 vbraun

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 jdemeyer

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 clean-up ticket, with almost no functional changes.

comment:14 Changed 6 years ago by vbraun

Well maybe it triggers a pre-existing 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 jdemeyer

How common is this failure?

comment:16 Changed 6 years ago by vbraun

Maybe 50% when the system is under load...

comment:17 Changed 6 years ago by jdemeyer

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 git

  • Commit changed from d556229dd9fec47e492a3543b0815ce02b4870de to c8d1a289e95184f3f6f5528e8fb8ee4638ab47c0

Branch pushed to git repo; I updated commit sha1. New commits:

c8d1a28Revert interrupt change

comment:19 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

I confirm the issue and this should fix it.

comment:20 Changed 6 years ago by rws

  • Milestone changed from sage-6.5 to sage-6.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 vbraun

  • Branch changed from u/jdemeyer/ticket/17718 to c8d1a289e95184f3f6f5528e8fb8ee4638ab47c0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.