This is an archive of the discontinued LLVM Phabricator instance.

Fix some portability issues in unittest2
ClosedPublic

Authored by zturner on Nov 5 2015, 2:12 PM.

Details

Reviewers
tfiala
Summary
TBH I'm honestly not sure what the problem was before, or why this fixes
it.  But what I can tell from debugging is that under Py3,
`sortMethodsUsing` is treated as a class attribute, but it was being
accessed as `self.sortMethodsUsing`.  What the full implications of this
are I don't quite know, but the symptom here was that the value we were
assigning to it -- the global lambda `cmp_` -- was being treated as a
bound method instead of a global method.  So it was expecting to be called
with a `self` argument.

Why exactly this happens in Python 3, and why exactly this *doesn't*
happen in Python 2 are not clear to me.

It's worth mentioning that I tried to switch unittest2 over to unittest -- the builtin implementation -- as a means of solving the Py2 / Py3 portability problems, and it was non-trivial at best, at least with my limited Python knowledge. The reason is that we have many custom decorators that rely on internals of unittest2 that are even documented in the source code as implementation details. But we use them, and they don't exist in the Python 3 version of unittest2, and as a result I don't know how to do the port smoothly.

Honestly, I couldn't even get it working under Python 2.7's built-in unittest implementation for similar reasons (relying on implementation details). It would be nice to do this someday, but our decorators have so many layers of wrapping and indirection that it makes my head explode trying to figure out what's going on :-/

Diff Detail

Event Timeline

zturner updated this revision to Diff 39416.Nov 5 2015, 2:12 PM
zturner retitled this revision from to Fix some portability issues in unittest2.
zturner updated this object.
zturner added a reviewer: tfiala.
zturner added a subscriber: lldb-commits.
tfiala accepted this revision.Nov 6 2015, 10:27 AM
tfiala edited edge metadata.

Looks reasonable. Do we know if it runs anywhere else yet?

This revision is now accepted and ready to land.Nov 6 2015, 10:27 AM

I'm probably being dense. But do we know if what runs anywhere else?

zturner closed this revision.Nov 6 2015, 1:40 PM
tfiala added a comment.Nov 6 2015, 1:42 PM

I meant I looked at it but didn't try to run it (on Linux or OS X), and was wondering if you had tried to run it on a non-Windows platform.

zturner added a subscriber: zturner.Nov 6 2015, 1:44 PM

Ahh, no sorry. There's nothing platform specific in the patch though, so I
don't anticipate any problems on that front. I did run it under 2.7 and it
worked, so I expect it will run under 2.7 on other platforms.