Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Py3 tests2 #913

Merged
merged 5 commits into from Oct 28, 2011
Merged

Py3 tests2 #913

merged 5 commits into from Oct 28, 2011

Conversation

takluyver
Copy link
Member

Fixes for a couple of tests under Python 3.

@ellisonbg, I'm having some trouble with a test in nbformat. With these changes, I get:

======================================================================
FAIL: test_roundtrip (IPython.nbformat.v2.tests.test_json.TestJSON)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/thomas/Code/virtualenvs/ipy-2to3/lib/python3.2/site-packages/ipython-0.12.dev-py3.2.egg/IPython/nbformat/v2/tests/test_json.py", line 18, in test_roundtrip
    self.assertEquals(reads(s),nb0)

followed by a long dump of the before and after dicts - I can't even spot the != in there, but I think the difference is between:

{'latex': '$a$', 'prompt_number': 3, 'svg': '<svg>', 'javascript': 'var i=0;', 'jpeg': 'ZGF0YQ==\n', 'json': 'json data', 'html': 'The HTML rep', 'text': '<array a>', 'output_type': 'pyout', 'png': 'ZGF0YQ==\n'}

and (N.B. b'data'):

{'latex': '$a$', 'prompt_number': 3, 'svg': '<svg>', 'javascript': 'var i=0;', 'jpeg': b'data', 'json': 'json data', 'html': 'The HTML rep', 'text': '<array a>', 'output_type': 'pyout', 'png': b'data'}

Any immediate thoughts?

@minrk
Copy link
Member

minrk commented Oct 22, 2011

There are definitely some bugs in the string/unicode/bytes logic in nbformat. In the JSON roundtrip, the problem is that the inverse of the b64 encode is never called. This doesn't come up, because the bytes are already b64-encoded in actual practice, and the bytes-logic in the JSONEncoder is actually never called in Python2 (as far as I can tell).

I think the real bug is that the base64_encode/decode functions in rwbase encode cell.png / cell.jpeg, but as far as I can tell, those attributes never exist. png/jpegs are attached to outputs of a cell, not the cells themselves.

This doesn't come up in Python2, because that output appears to always already be b64-encoded bytes, which pass right through to/from JSON without issue.

@takluyver
Copy link
Member Author

Is there anything simple that can be done to fix it? I'm not at all familiar with this part of the code.

@minrk
Copy link
Member

minrk commented Oct 22, 2011

Further detail: every bytes object in the notebook is already a b64-encoded bytes object. So, instead of performing b64 encoding/decoding around JSON, it should be an ascii encoding/decoding, leaving the contents unchanged.

@minrk
Copy link
Member

minrk commented Oct 22, 2011

Yes, I've got it working locally - the answer is to stop using b64 to/from JSON, as it has already been encoded. I'll PR to your branch shortly with fixes after I clean up all my code.

base64 encoding functions were called, but had no effect, because
the notebook already has everything as b64-encoded bytestrings, which
are valid ascii literals on Python 2.

However, the encode/decode logic is actually triggered on Python 3, revealing its errors.

This fixes the base64 functions that had no effect to have their intended effect,
but does not use them.  Rather, it is assumed that
bytes objects are already b64-encoded (and thus ascii-safe), which
assumption was already made in Python 2.
@minrk
Copy link
Member

minrk commented Oct 22, 2011

PR issued

@takluyver
Copy link
Member Author

Thanks Min - I've added that to this PR, and the nbformat tests are passing now.

try:
src.encoding = None # IPython expects stdin to have an encoding attribute
except Exception:
pass # ...but it's a read-only attribute in Python 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be 'if not hasattr', rather than try/except? We don't need to do it if it already exists.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Done.

@takluyver
Copy link
Member Author

I've also made a couple of decorators to mark known failures, so it's easier to spot regressions. The parallel package is still failing - I'll try to look into it if I get time, but I think it's fairly low priority.

@fperez
Copy link
Member

fperez commented Oct 28, 2011

This is looking pretty solid, so I'll merge it now to avoid bitrot. When you get a chance to tackle the parallel ones, a new PR can come in for those. Thanks for this work! Looking better and better on py3 :)

fperez added a commit that referenced this pull request Oct 28, 2011
Fix more tests under Python3.  The test suite isn't fully fixed yet under py3, but this gets us closer.
@fperez fperez merged commit 9463121 into ipython:master Oct 28, 2011
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Fix more tests under Python3.  The test suite isn't fully fixed yet under py3, but this gets us closer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants