One doctest in cmdline.py is too touchy with respect to the shebang #14365

SnarkBoojum · 2013-03-27T08:56:34Z

In cmdline.py, one of the doctest checks the exact shebang of ipython ; the attached patch checks there is a shebang trying to run python, but doesn't check exactly how python is run.

CC: @kiwifb

Component: doctest coverage

Keywords: packaging

Author: Julien Puydt

Reviewer: François Bissey, Julien Puydt

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

SnarkBoojum · 2013-03-27T08:57:16Z

Patch

SnarkBoojum · 2013-03-27T08:57:28Z

comment:1

Attachment: cmdline.patch.gz

nexttime · 2013-03-27T12:46:26Z

Reviewer: Leif Leonhardy

nexttime · 2013-03-27T12:46:26Z

comment:2

Well, your patch doesn't really fit


    Check that ``sage-location`` did its job in making Python scripts
    relative.  We test it on the ``ipython`` script::

Furthermore, for Sage-on-foo, there might not even be a $SAGE_LOCAL/bin/ipython [script], so I think the whole test should be changed.

nexttime · 2013-03-27T12:46:26Z

Changed keywords from none to packaging

nexttime · 2013-03-27T12:46:26Z

Author: Julien Puydt

SnarkBoojum · 2013-03-27T16:27:15Z

comment:3

Should I rewrite the patch to just remove that test?

nexttime · 2013-03-27T16:33:29Z

comment:4

Replying to @SnarkBoojum:

Should I rewrite the patch to just remove that test?

Just take another script from $SAGE_LOCAL/bin/ which is unlikely to use some system-wide version / differ in "repackaged" Sage releases?

Why at all did the test fail for you?

SnarkBoojum · 2013-03-27T16:36:31Z

comment:5

In debian, ipython starts with:

  #! /usr/bin/python

nexttime · 2013-03-27T17:21:38Z

comment:6

Replying to @SnarkBoojum:

In debian, ipython starts with:
  #! /usr/bin/python

Which is just stupid. Orthogonal to that, why should Debian's ipython script be in $SAGE_LOCAL/bin/?

kiwifb · 2013-03-27T17:26:04Z

comment:7

But has in sage-on-gentoo SAGE_LOCAL is probably set to /usr. cmdline.py is full of stuff that fail on sage-on-gentoo a lot of them we just don't really think of fixing.

nexttime · 2013-03-27T17:35:03Z

comment:8

Replying to @kiwifb:

But has in sage-on-gentoo SAGE_LOCAL is probably set to /usr. cmdline.py is full of stuff that fail on sage-on-gentoo a lot of them we just don't really think of fixing.

IMHO Sage just shouldn't contain such too specific tests.

SnarkBoojum · 2013-03-27T17:44:24Z

comment:9

Sage contains :

test for features
test for very specific tests (on places of files, on very specific version of various things)

The first are important. The others wouldn't be a problem if they were all in the same file, which distributions could then not package and not suffer through. But they're scattered everywhere!

kiwifb · 2013-03-27T20:22:26Z

comment:10

Just for fun that's what fails in sage-on-gentoo after we patch most of what we care of

sage -t -long "devel/sage-main/sage/tests/cmdline.py"       
**********************************************************************
File "/usr/share/sage/devel/sage-main/sage/tests/cmdline.py", line 196:
    sage: ret
Expected:
    0
Got:
    1
**********************************************************************
File "/usr/share/sage/devel/sage-main/sage/tests/cmdline.py", line 217:
    sage: print out
Expected:
    Found package sqlalchemy in spkg/standard/sqlalchemy-...spkg
    = SQLAlchemy =
    ...
    SQLAlchemy is the Python SQL toolkit...
Got:
    sage-run received unknown option: --info 
    usage: sage [options]
    Try 'sage -h' for more information.
    <BLANKLINE>
**********************************************************************
File "/usr/share/sage/devel/sage-main/sage/tests/cmdline.py", line 224:
    sage: ret
Expected:
    0
Got:
    1
**********************************************************************
File "/usr/share/sage/devel/sage-main/sage/tests/cmdline.py", line 347:
    sage: print err
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: refusing to run doctests...
Got:
    <BLANKLINE>
**********************************************************************
File "/usr/share/sage/devel/sage-main/sage/tests/cmdline.py", line 352:
    sage: print err
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: refusing to run doctests...
Got:
    Traceback (most recent call last):
      File "/usr//bin/sage-ptest", line 446, in <module>
        p = multiprocessing.Pool(numthreads)
      File "/usr/lib64/python2.7/multiprocessing/__init__.py", line 232, in Pool
        return Pool(processes, initializer, initargs, maxtasksperchild)
      File "/usr/lib64/python2.7/multiprocessing/pool.py", line 129, in __init__
        raise ValueError("Number of processes must be at least 1")
    ValueError: Number of processes must be at least 1
    <BLANKLINE>
**********************************************************************

I'd like to dig the problem with the value from "ret" in 2 tests. The sqlalchemy tests really duplicate testing in another part of sage I am sure. My solution for that will be to eventually remove it surgically since the only sage spkg handling functionality that it is working and intend to keep is to produce spkg.

SnarkBoojum · 2013-03-27T21:52:57Z

comment:11

Oh, that one:

File "/usr/share/sage/devel/sage-main/sage/tests/cmdline.py", line 347:
    sage: print err
Expected:
    Traceback (most recent call last):
    ...
    RuntimeError: refusing to run doctests...
Got:
    <BLANKLINE>

I have two like this in cmdline.py and another one in control.py.

And I have a few other places where sage expects an error and doesn't... like a place where it deliberately asks something about a polytope in dimension 7 to a palp program which is supposed to work in dimension at most 6... as debian's (future) palp package goes up to eleven [because sage goes as far], the test fails because the computation actually works.

But let's get back to this ticket : what do you propose it should test? I don't think there is such a thing as "another script from $SAGE_LOCAL/bin/ which is unlikely to use some system-wide version / differ in "repackaged" Sage releases" as Leif put it...

kiwifb · 2013-03-27T22:10:44Z

comment:12

To be brutally honest this file probably can be completely dropped from a binary distro. It is only testing that the various options of the sage command line behave as expected. There is no functionality in here.

jdemeyer · 2013-04-28T22:12:15Z

comment:13

The patch simply destroys the whole point of the test, so it surely isn't acceptable.

jdemeyer · 2013-04-28T22:13:46Z

comment:14

Replying to @SnarkBoojum:

I have two like this in cmdline.py and another one in control.py.

That's because upstream Python very stupidly refuses to add a patch fixing an important security issue.

jdemeyer · 2013-04-28T22:18:19Z

comment:15

Replying to @SnarkBoojum:

Sage contains :

test for features

test for very specific tests (on places of files, on very specific version of various things)

In reality there is also a lot of
3. tests combining a type 1 and a type 2 test

The test that this ticket is about is such a test. If it was only type 2 we could just remove it. But it's really meant as a test of type 1, namely that sage-location works.

kiwifb · 2013-05-01T10:41:43Z

comment:16

Actually that's one of the tests we now keep in sage-on-gentoo and I don't really have a problem with it - lucky. For a binary distro you may want to drop some tests. I don't see the point in shipping and testing sage-location in that particular case. I am not talking about a bdist, I am talking about something installed by your distro package manager.

kiwifb · 2013-05-02T01:45:54Z

comment:17

Well that's interesting, I will drop the test in question shortly from sage-on-gentoo. We now have

#!/usr/bin/python-exec-c\n

which comes from the new way to handle multiple simultaneous python install at the time in Gentoo. Like I said, it won't be missed if it just test that sage-relocation did its work. We don't do that in sage-on-gentoo anyway. And I am sure you don't want to do that either in a debian package that install sage as part of the system.

While it would be nice to have sage upstream completely distro friendly, there are some divergences at the moment because of slightly different objectives. I am absolutely ready to review this as invalid. As long as sage upstream support something like "sage-relocation" it will make sense to test it. And it won't ever for a project like sage-on-foo.

If you have a sure to work, in both world, alternative to the test I would be ok with it. In the meantime I am not really willing to spend much time on it.

SnarkBoojum · 2015-03-19T06:19:32Z

comment:22

All of this discussion is outdated : debian will patch out any test which doesn't make sense.

I suggest to close as invalid/outdated/obsolete (whatever the trac name is), thanks.

kiwifb · 2015-03-19T06:27:59Z

comment:23

Set to invalid.

nexttime · 2015-03-19T14:48:41Z

Changed reviewer from Leif Leonhardy to François Bissey, Julien Puydt

nexttime · 2015-03-19T14:48:41Z

comment:24

Replying to @kiwifb:

Set to invalid.

I'm interpreting this as "sudo set to invalid", although that's not necessary here.

SnarkBoojum mannequin added this to the sage-5.11 milestone Mar 27, 2013

SnarkBoojum mannequin added t: tests labels Mar 27, 2013

SnarkBoojum mannequin assigned sagetrac-mvngu Mar 27, 2013

SnarkBoojum mannequin added the s: needs review label Mar 27, 2013

jdemeyer added s: needs work and removed s: needs review labels Apr 28, 2013

jdemeyer removed this from the sage-5.11 milestone Aug 13, 2013

jdemeyer added this to the sage-5.12 milestone Aug 13, 2013

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014

SnarkBoojum mannequin added s: needs review and removed s: needs work labels Mar 19, 2015

kiwifb added s: positive review and removed s: needs review labels Mar 19, 2015

nexttime mannequin removed this from the sage-6.4 milestone Mar 19, 2015

vbraun removed the s: positive review label Mar 21, 2015

vbraun closed this as completed Mar 21, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

One doctest in cmdline.py is too touchy with respect to the shebang #14365

One doctest in cmdline.py is too touchy with respect to the shebang #14365

SnarkBoojum mannequin commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

kiwifb commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

kiwifb commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

kiwifb commented Mar 27, 2013

jdemeyer commented Apr 28, 2013

jdemeyer commented Apr 28, 2013

jdemeyer commented Apr 28, 2013

kiwifb commented May 1, 2013

kiwifb commented May 2, 2013

SnarkBoojum mannequin commented Mar 19, 2015

kiwifb commented Mar 19, 2015

nexttime mannequin commented Mar 19, 2015

nexttime mannequin commented Mar 19, 2015

One doctest in cmdline.py is too touchy with respect to the shebang #14365

One doctest in cmdline.py is too touchy with respect to the shebang #14365

Comments

SnarkBoojum mannequin commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

kiwifb commented Mar 27, 2013

nexttime mannequin commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

kiwifb commented Mar 27, 2013

SnarkBoojum mannequin commented Mar 27, 2013

kiwifb commented Mar 27, 2013

jdemeyer commented Apr 28, 2013

jdemeyer commented Apr 28, 2013

jdemeyer commented Apr 28, 2013

kiwifb commented May 1, 2013

kiwifb commented May 2, 2013

SnarkBoojum mannequin commented Mar 19, 2015

kiwifb commented Mar 19, 2015

nexttime mannequin commented Mar 19, 2015

nexttime mannequin commented Mar 19, 2015