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