Opened 3 years ago

Closed 3 years ago

#25212 closed defect (duplicate)

Fix malformatted test in sage.repl.ipython_extension

Reported by: embray Owned by:
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: misc Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers:
Report Upstream: N/A Work issues:
Branch: u/embray/misc/cython-cell-magic-test (Commits, GitHub, GitLab) Commit: 3cd27b83ef288696c7fc70eb5419be1085a3d175
Dependencies: Stopgaps:

Status badges

Description

For some reason that I don't yet understand, on the main develop branch of Sage, the doctest runner is not complaining about this test even though it's syntactically malformatted. Whereas on my Python 3 branch it is correctly reported as a syntax error.

Change History (11)

comment:1 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 3 years ago by tscrim

Shouldn't the test be:

sage: shell.run_cell('f()')

Admittedly, I don't know exactly what is going on, but it looks like a different test.

comment:3 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:4 Changed 3 years ago by slelievre

In other words, the right fix might be to replace

            ....: shell.run_cell('f()')

by

            sage: shell.run_cell('f()')

instead of replacing it by

            sage: f()

comment:5 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

It actually isn't. I fact the latter won't work (it produces a NameError). I'm not exactly sure why that is the case--I don't know how get_test_shell handles namespaces. But I had that thought at first two and it does not work. Compare also to the %%fortran test in the same module.

comment:6 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_info

I guess you are defeating the whole purpose of the test then....

comment:7 Changed 3 years ago by embray

It's not, nor do I think you're correctly interpreting the purpose of the test.

I think the issue is this: cython_compile injects its result directly into the actual global namespace (user_globals). get_test_shell() meanwhile implements a fake IPython shell for testing features thereof, but takes care not to modify the actual global namespace this appears to not exactly be true, but there's still something going on with how cython_compile() uses user_globals, and similarly with fortran I think.

That said, all the test is testing in this case is that %%cython magic works, like, syntactically. So I think it's beside the point.

Last edited 3 years ago by embray (previous) (diff)

comment:8 Changed 3 years ago by embray

  • Status changed from needs_info to needs_review

comment:9 Changed 3 years ago by embray

  • Status changed from needs_review to needs_info

Ok, I take it back. It seems like this does work at the command-line, but it does not work correctly in the doctests themselves. So there's a weird interaction between the doctest runner, the test shell, and user_globals.

I think that problem must have existed for a while though considering that

a) This test was already broken for a long time but not being run.

and

b) The other similar test in this module seems to already be working around the same issue.

comment:10 Changed 3 years ago by embray

  • Milestone changed from sage-8.3 to sage-8.4

I believe this issue can reasonably be addressed for Sage 8.4.

comment:11 Changed 3 years ago by jdemeyer

  • Milestone changed from sage-8.4 to sage-duplicate/invalid/wontfix
  • Resolution set to duplicate
  • Status changed from needs_info to closed

Fixed in #26038.

Note: See TracTickets for help on using tickets.