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

[Pavel Labath via lldb-commits]

labath added a comment.

LGTM from my side after the two fixes in `no_debug_info_test` and `skipUnlessListedRemote`


================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:555
@@ -554,3 +554,3 @@
     # Mark this function as such to separate them from the regular tests.
-    wrapper.__no_debug_info_test__ = True
-    return wrapper
+    func.__no_debug_info_test__ = True
+    return func
----------------
This will not cause the class to be skipped, but it will still not achive the desired effect when used on a class. Please leave the safeguard in.

I like the simplification of the wrapper btw.

================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:560
@@ -559,3 +559,1 @@
     """Decorate the item as a debugserver test."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@debugserver_test can only be used to decorate a test method")
----------------
I would very much welcome this behavior, however not all of our decorators are ready for that. The point is not that the class would be skipped if the condition is true (this is what we want), the problem is that the class would be skipped **even when the condition is false**. So, putting a wrong kind of a decorator on a class causes the whole class to be ignored (because what the decorator returns is not a python class, which means unittest cannot find the test methods inside), and reduces the test coverage for everyone. We've already had a case when a test wasn't running for several months before I noticed (see r257901) that it had a bad decorator applied at the class level.   

The test you mention, `TestAddDsymCommand.py`, is fine because it eventually calls into `unittest2.skipUnless`, which knows how to handle this case properly, but skipTestIfFn is bad because it returns a function-like object instead of a class. Also, all our XFAIL decorators are bad, and afaik not even unittest has the support for xfail at class level.

So if you can make the decorators work properly on classes (in case of skip, you probably just need to route everything to unittest.skipIf, but it's not going to be that trivial in the case of xfails), I am all for using them, but until then, the exception safeguard needs to stay.

================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:824
@@ +823,3 @@
+        else:
+            return (True, "skipping because remote is not listed")
+    return skipTestIfFn(is_remote_unlisted)
----------------
Actually, the original behavior of the decorator was to *not* skip the test when the test is *not* remote, so please put `False` here for now. 
However, I find the whole decorator weird and I'll get rid of it completely after you are done here.

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


http://reviews.llvm.org/D16741