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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
