Further clean-up of expect.py

[jdemeyer]

Depends on #17704

Component: interfaces

Author: Jeroen Demeyer

Branch/Commit: c8d1a28

Reviewer: Vincent Delecroix, Marc Mezzarobba, Ralf Stephan

Issue created by migration from https://trac.sagemath.org/ticket/17718

[jdemeyer]

Branch: u/jdemeyer/ticket/17718

[jdemeyer]

Commit: d556229

[jdemeyer]

New commits:

8501bb1	Clean up in expect.py
d556229	Further clean-up of interfaces

[videlec]

comment:3

Why

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

If this is necessary, could you add a doctest?

[jdemeyer]

comment:4

Replying to @videlec:

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')

[mezzarobba]

Changed commit from d556229 to 568b6a8

[mezzarobba]

Changed branch from u/jdemeyer/ticket/17718 to u/mmezzarobba/17718-expect

[jdemeyer]

comment:6

Why that extra commit?

[mezzarobba]

comment:7

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.

[jdemeyer]

comment:8

Replying to @mezzarobba:

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.

[jdemeyer]

Changed commit from 568b6a8 to d556229

[jdemeyer]

comment:9

In any case, this could be discussed in a different ticket.

[jdemeyer]

Changed branch from u/mmezzarobba/17718-expect to u/jdemeyer/ticket/17718

[mezzarobba]

comment:10

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.

[mezzarobba]

Reviewer: Vincent Delecroix, Marc Mezzarobba

[vbraun]

comment:11

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

[vbraun]

comment:12

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")

[jdemeyer]

comment:13

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.

[vbraun]

comment:14

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.

[jdemeyer]

comment:15

How common is this failure?

[vbraun]

comment:16

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

[jdemeyer]

comment:17

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.

[sagetrac-git]

Changed commit from d556229 to c8d1a28

[sagetrac-git]

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

c8d1a28

Revert interrupt change

[jdemeyer]

comment:19

I confirm the issue and this should fix it.

[rwst]

comment:20

Patchbot says fine. Tests in interfaces on my machine pass too. As the code was already reviewed by others I dare to set positive.

[rwst]

Changed reviewer from Vincent Delecroix, Marc Mezzarobba to Vincent Delecroix, Marc Mezzarobba, Ralf Stephan

[vbraun]

Changed branch from u/jdemeyer/ticket/17718 to c8d1a28