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

[Zachary Turner via lldb-commits]

zturner added a comment.

> If we haven't already, we should probably have some kind of exception wrapper around our decorators that catches non-unittest-related (i.e. unexpected) exceptions and somehow makes them more prevalent - maybe a hard error on the test or an abort or something.


Yea, part of why I wanted to do this work in the first place is because I want to make some sweeping changes to all decorators as well, and it's impossible when everything is calling directly into `unittest2` or just doing its own thing in whatever way.  So having the code all in one place makes this very easy.


================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:1102
@@ -1164,2 +1101,3 @@
+                return func(*args, **kwargs)
         return wrapper
 
----------------
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.


http://reviews.llvm.org/D16741