Opened 9 years ago

Closed 9 years ago

#12911 closed defect (fixed)

fix failing ipython test in tests/cmdline

Reported by: jhpalmieri Owned by: jason
Priority: major Milestone: sage-5.1
Component: misc Keywords: ipython
Cc: Merged in: sage-5.1.beta6
Authors: John Palmieri Reviewers: Keshav Kini
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jhpalmieri)

The test

        sage: (out, err, ret) = test_executable(["sage", "--ipython"], "\n3**33\n")
        sage: out.find("5559060566555523") >= 0
        True

fails if people have installed a newer version (0.12?) of ipython which leaves incompatible files in ~/ipython/. We might consider setting IPYTHONDIR to $DOT_SAGE/ipython/ in sage-env, but the simplest fix seems to be to do this right before running the test.

Test this by running sage --ipython to create config files in ~/ipython/. Then edit ~/ipython/ipy_user_conf.py to (for example) make it invalid Python, so it prints an error when you start sage --ipython. Then doctest the file cmdline.py before and after applying the patch.

Attachments (1)

trac_12911-ipython-test.patch (891 bytes) - added by jhpalmieri 9 years ago.

Download all attachments as: .zip

Change History (12)

Changed 9 years ago by jhpalmieri

comment:1 Changed 9 years ago by jhpalmieri

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 9 years ago by jason

So this ticket depends on #12719, right?

comment:3 Changed 9 years ago by jason

Oh, wait; rereading your description makes it seem like the problem is if they have a system 0.12, but Sage's 0.10, that causes a problem.

So installing #12719 won't solve the problem by itself, since modifying the global config file to be invalid python still would cause problems. However, perhaps the IPython config directory should be set to a temporary directory that can be deleted after the doctest is run? That would fulfill the ideal that doctests don't leave files laying around.

comment:4 Changed 9 years ago by jhpalmieri

The doctest shouldn't leave any extra files lying around: on startup, Sage should create DOT_SAGE/ipython/ (if it doesn't exist already) and populate it with various files. From the main sage script (SAGE_ROOT/spkg/bin/sage):

    IPYTHONDIR="$DOT_SAGE/ipython" && export IPYTHONDIR
    IPYTHONRC="ipythonrc" && export IPYTHONRC
    if [ ! -d "$IPYTHONDIR" ]; then
        mkdir -p "$DOT_SAGE"
        cp -r "$SAGE_ROOT/ipython" "$DOT_SAGE/"
    fi

Running the doctest shouldn't create anything new, just use the already-present config files.

comment:5 Changed 9 years ago by kini

  • Reviewers set to Keshav Kini
  • Status changed from needs_review to positive_review

Oops, I didn't realize this hadn't been reviewed! The code is obviously correct. Positive review from me.

comment:6 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Why not do this in sage-env? That would seem a lot more logical to me.

comment:7 follow-up: Changed 9 years ago by jhpalmieri

I think if you run sage --ipython, it should not use the files in DOT_SAGE/ipython; otherwise, the default IPython prompt becomes sage:, for example.

comment:8 Changed 9 years ago by kini

I agree with John, for what it's worth.

comment:9 in reply to: ↑ 7 Changed 9 years ago by jdemeyer

  • Status changed from needs_info to needs_review

Replying to jhpalmieri:

I think if you run sage --ipython, it should not use the files in DOT_SAGE/ipython; otherwise, the default IPython prompt becomes sage:, for example.

Cool. In this case, what you did is the right thing.

comment:10 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Why can't I directly change status from needs_info to positive_review? I always find this so annoying.

comment:11 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.1.beta6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.