-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fix some tests for Python 3.3 #1748
Conversation
Isolated one (potential) issue in importlib. Commit a894e07 fixes what was causing the error from our side, and I've sent an issue to the Python bug tracker too: http://bugs.python.org/issue14846 |
Test results for commit a894e07 merged into master
Not available for testing: |
Test results for commit a894e07 merged into master
Not available for testing: |
The failure the first time seems to be one of those intermittent failures in parallel we see from time to time, not related to this issue. |
Test results for commit 2873d08 merged into master
Not available for testing: python2.6 |
All tests are passing on Python 3.3 now, but setting up a virtualenv still doesn't work straight off, so I haven't added it to the PR test script yet. |
Thanks! Seems to me like doctests are more trouble than they are worth. |
Tell me about it ;-) |
Rebased because of conflicts after #1732 was merged. |
And all tests are passing again on Python 3.3. |
@@ -160,7 +160,7 @@ class defaults. | |||
final_help = [] | |||
final_help.append(u'%s options' % cls.__name__) | |||
final_help.append(len(final_help[0])*u'-') | |||
for k,v in cls.class_traits(config=True).iteritems(): | |||
for k,v in sorted(cls.class_traits(config=True).iteritems()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this adds cost to all instances only for the benefit of a doctest. Instead, the test should be changed, to not depend on the order (it can do a set
check on both keys and values, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this affects how the options for help are printed - I think it's awkward to have the options displayed in a random order each time you do something like ipython --help-all
.
Given that this method is only called to display help information (class_get_help()
), I think the cost of Python's built in sort is going to be negligible compared to the speed with which output can be written. This was probably the place I was most confident that sorting was the right way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't realize this was inside of a method called for interactive help, sorry. For some reason I thought it was in the constructor itself and that it would thus affect everything all the time.
Yes, I agree that if it's only invoked interactively at help time, it's OK. Sorry for the misfire.
OK, it turns out my review was 100% pure noise, zero signal! A new record :) This is otherwise great, merging. Thanks for the good work! |
Thanks, Fernando. I'd rather explain some of the changes than have them go unreviewed, so I'm not complaining. Hopefully that will improve the numbers on ShiningPanda, but there are also some test failures I don't see on my local installation. If they continue, I'll see if they go when ShiningPanda gets the next 3.3 alpha before I investigate them. |
On Tue, May 29, 2012 at 4:37 PM, Thomas Kluyver
Excellent, thanks! I'm thrilled to know that we'll be ready for 3.3 |
Fix some tests for Python 3.3.
Update: All tests are now passing on Python 3.3.
The main change is that, for security reasons, the ordering of dicts is unpredictable. It's never something we should have been relying on, but various doctests did depend on the order.
Most of the remaining test failures seem to be problems with importing things - I'll look more carefully at the documentation, but I think there might be bugs in the new implementation of import.