[Lldb-commits] [PATCH] D16741: Make many of the skip decorators use common code

[Pavel Labath via lldb-commits]

labath added inline comments.

================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:1102
@@ -1164,2 +1101,3 @@
+                return func(*args, **kwargs)
         return wrapper
 
----------------
zturner wrote:
> labath wrote:
> > zturner wrote:
> > > labath wrote:
> > > > This return statement is the root cause of the problem. If `func` is a class, you will replace it by a strange function-like object.
> > > This return statement is newly added anyway (and looks to be a mistake).  Does this mean if I remove the return statement, the all of the skip decorators will be able to be used at class level?
> > > 
> > I'm not exactly sure what you mean. You can't simply remove the return statement, as that would make the decorator not work. I would try to make this decorator call `unittest2.skipIf`, which knows how to do the right thing. So, something like
> > ```
> > def skipTestIfFn(expected_fn, bugnumber=None):
> >   skip, reason = ...
> >   return unittest2.skipIf(skip, reason)
> > ```
> > would work (I think), but you could run into some complications as now `expected_fn` gets evaluated at decorator application time rather than during test invocation.
> > 
> > You can try this out for your self. Remove the exception guard, apply the decorator on a class with the condition that is false, and make sure that the tests *do* run.
> > 
> > But maybe we should do that as a separate patch ?
> Doing it in a separate patch makes sense.  In any case, my point here is that this code was already not returning the result of calling the function (just look at the left side of the diff).  I think when we get here `func` is always the underlying function (i.e. the actual test function), and those don't return anything, they just run the test.
> 
> I would have to step through it in a debugger to really be sure though, the whole decorator model is still kind of hard to wrap my head around with the numerous layers of nested functions.
Ok, I see what you mean now. The `return` in `return func(...)` is probably not needed, but it definitely doesn't hurt either. The problem is one level up, in `skipTestIfFn_impl`. Whatever is the result of `skipTestIfFn_impl(X)` gets used instead of the real `X`. So if `X` is the whole class then you must return something that looks like a class there. Alas, we are returning `wrapper`, which is a function, and things go downhill from there...


http://reviews.llvm.org/D16741