Make many of the skip decorators use common code

[zturner]

Summary

Trying to reduce the complexity of all our decorators, I decided to make most of them use skipTestIfFn. Unfortunately there's some issues with my patch. I'm not really an expert with all these decorators and multiple levels of decorating, but something about that is causing some issues.

For example, I get this now when running the test suite:

UNEXPECTED SUCCESS: test_Hc_then_Csignal_signals_correct_thread_launch_debugserver_dwarf (tools\lldb-server\TestLldbGdbServer.py)
UNEXPECTED SUCCESS: test_with_attach_to_process_with_id_api_dwarf (python_api\hello_world\TestHelloWorld.py)
UNEXPECTED SUCCESS: test_with_attach_to_process_with_name_api_dwarf (python_api\hello_world\TestHelloWorld.py)

But the tests are still failing. Would someone mind helping me figure out what's wrong here?

[labath]

I am confused as to how could this even work, since all your decorators seem to be ignoring the function they are decorating. I'm pretty sure this is going to cause some very weird things to happen things to come out of the decoration process, which might explain your confusion. Try giving it a go by passing the function explicitly.

packages/Python/lldbsuite/test/lldbtest.py

560

Please don't remove these. They make sure that you do not attempt to apply this decorator to a class. If the decorator is not ready for that (and the current skipIfTestFn does not appear to be), it will just cause the whole cause the whole class to be silently ignored (we've had this happen before).

I think it's ok to centralize the check in skipTestIfFn.

562

You have dropped "func" completely here. Should this be skipTestIfFn(should_skip_debugserver_tests, func)? (Same goes for other decorators.)

[zturner]

packages/Python/lldbsuite/test/lldbtest.py

560

Why don't we want this behavior? With this check, an exception is being thrown if the condition is true. Currently the test suite is not failing anywhere with this exception, meaning the check is never true. And it would only be true if we put the decorator on the actual class. Which we already do in some places (see TestAddDsymCommand.py) and is actually a very useful way of skipping or xfailing an entire class. I want to be able to skip / xfail entire classes *more* easily, not less easily.

[zturner]

I guess we need to be consistent. Do we want to be able to decorate entire classes, or not? If we do, then we should remove the check. If we don't then we should leave the check. But since many tests are already using these decorators at the class level (TestEvents.py, TestAddDsymCommand.py, TestObjCMethods2.py, TestLoadUnload.py, and many more) those decorators cannot yet be ported to using common code until we decide which we want.

[zturner]

I think this should fix most of the issues. A few of the decorators I wasn't able to touch because of the issues with class-level decorators. But I think this is a good start. My test statistics before and after this patch are the same.

If someone else could run test suite before and after this patch and let me know how the numbers look on your platform I would appreciate it.

Suggestions welcome.

[tfiala]

I'll see if I can run this tonight. If not tonight, I'll do it first
thing in the morning.

[labath]

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

packages/Python/lldbsuite/test/lldbtest.py

555

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.

560

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.

824

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.

1102

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.

[zturner]

packages/Python/lldbsuite/test/lldbtest.py

1102

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?

[tfiala]

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.

I've been thinking about this in the background lately. Several times over
the last couple years I found decorator logic that had errors in them
(literally, the code was throwing an exception within the decorator logic,
such as using a module that isn't imported), but this will go by silently
unless some work is put into logging errors there.

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.

[labath]

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.

+1

In the new test runner, the raise Exception thing will not actually make the test as a whole fail, but at least it will generate a lot of noise in the output, which will hopefully make someone notice it, so I guess it's better than nothing...

packages/Python/lldbsuite/test/lldbtest.py

1102

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 ?

[zturner]

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.

packages/Python/lldbsuite/test/lldbtest.py

1102

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.

[labath]

packages/Python/lldbsuite/test/lldbtest.py

1102

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...

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_in_delayslot/TestAvoidBreakpointInDelaySlot.py

 import lldb
 import lldbsuite.test.lldbutil as lldbutil
 from lldbsuite.test.lldbtest import *
 
 class AvoidBreakpointInDelaySlotAPITestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
-    @skipUnlessArch(archs=re.compile('mips*'))
+    @skipUnlessArch(archs=[re.compile('mips*')])
     def test(self):
         self.build()
         exe = os.path.join(os.getcwd(), "a.out")
         self.expect("file " + exe,
                     patterns = [ "Current executable set to .*a.out.*" ])
         
         # Create a target by the debugger.
         target = self.dbg.CreateTarget(exe)

packages/Python/lldbsuite/test/lldbtest.py

"""Decorate the item as a test what don't use any debug info. If this annotation is specified
        then the test runner won't generate a separate test for each debug info format. """
     if isinstance(func, type) and issubclass(func, unittest2.TestCase):
         raise Exception("@no_debug_info_test can only be used to decorate a test method")
     @wraps(func)
     def wrapper(self, *args, **kwargs):
         return func(self, *args, **kwargs)
 
     # Mark this function as such to separate them from the regular tests.
     wrapper.__no_debug_info_test__ = True
     return wrapper
 
 def debugserver_test(func):
     """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")
-    @wraps(func)
-    def wrapper(self, *args, **kwargs):
-        if configuration.dont_do_debugserver_test:
-            self.skipTest("debugserver tests")
-        return func(self, *args, **kwargs)
-
-    # Mark this function as such to separate them from the regular tests.
-    wrapper.__debugserver_test__ = True
-    return wrapper
+    def should_skip_debugserver_tests():
+        return (configuration.dont_do_debugserver_test, "debugserver tests")
+    return skipTestIfFn(should_skip_debugserver_tests)
 
 def llgs_test(func):
     """Decorate the item as a lldb-server test."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@llgs_test can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(self, *args, **kwargs):
-        if configuration.dont_do_llgs_test:
-            self.skipTest("llgs tests")
-        return func(self, *args, **kwargs)
-
-    # Mark this function as such to separate them from the regular tests.
-    wrapper.__llgs_test__ = True
-    return wrapper
+    def should_skip_llgs_tests():
+        return (configuration.dont_do_llgs_test, "llgs tests")
+    return skipTestIfFn(should_skip_llgs_tests)
 
 def not_remote_testsuite_ready(func):
     """Decorate the item as a test which is not ready yet for remote testsuite."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@not_remote_testsuite_ready can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(self, *args, **kwargs):
-        if lldb.remote_platform:
-            self.skipTest("not ready for remote testsuite")
-        return func(self, *args, **kwargs)
-
-    # Mark this function as such to separate them from the regular tests.
-    wrapper.__not_ready_for_remote_testsuite_test__ = True
-    return wrapper
+    def is_remote():
+        return (lldb.remote_platform, "Not ready for remote testsuite")
+    return skipTestIfFn(is_remote)
 
 def expectedFailure(expected_fn, bugnumber=None):
     def expectedFailure_impl(func):
         if isinstance(func, type) and issubclass(func, unittest2.TestCase):
             raise Exception("Decorator can only be used to decorate a test method")
         @wraps(func)
         def wrapper(*args, **kwargs):
             from unittest2 import case
if six.callable(bugnumber):
         return expectedFailure_impl(bugnumber)
     else:
         return expectedFailure_impl
 
 # You can also pass not_in(list) to reverse the sense of the test for the arguments that
 # are simple lists, namely oslist, compiler, and debug_info.
 
 def not_in(iterable):
-    return lambda x : x not in iterable
+    def check_no_items_match(x):
+        for item in iterable:
+            if isinstance(item, re._pattern_type):
+                if re.match(item, x):
+                    return False
+            else:
+                if item == x:
+                    return False
+        return True
+    return check_no_items_match
 
 def check_list_or_lambda(list_or_lambda, value):
     if six.callable(list_or_lambda):
         return list_or_lambda(value)
     elif isinstance(list_or_lambda, list):
         for item in list_or_lambda:
             if value in item:
                 return True

 def expectedFlakeyGcc(bugnumber=None, compiler_version=None):
     return expectedFlakeyCompiler('gcc', compiler_version, bugnumber)
 
 def expectedFlakeyAndroid(bugnumber=None, api_levels=None, archs=None):
     return expectedFlakey(matchAndroid(api_levels, archs), bugnumber)
 
 def skipIfRemote(func):
     """Decorate the item to skip tests if testing remotely."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@skipIfRemote can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(*args, **kwargs):
-        from unittest2 import case
-        if lldb.remote_platform:
-            self = args[0]
-            self.skipTest("skip on remote platform")
-        else:
-            func(*args, **kwargs)
-    return wrapper
+    def is_remote():
+        return (lldb.remote_platform, "skip on remote platform")
+    return skipTestIfFn(is_remote)
 
 def skipUnlessListedRemote(remote_list=None):
-    def myImpl(func):
-        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-            raise Exception("@skipIfRemote can only be used to decorate a "
-                            "test method")
-
-        @wraps(func)
-        def wrapper(*args, **kwargs):
-            if remote_list and lldb.remote_platform:
-                self = args[0]
-                triple = self.dbg.GetSelectedPlatform().GetTriple()
-                for r in remote_list:
-                    if r in triple:
-                        func(*args, **kwargs)
-                        return
-                self.skipTest("skip on remote platform %s" % str(triple))
-            else:
-                func(*args, **kwargs)
-        return wrapper
-
-    return myImpl
+    def is_remote_unlisted(self):
+                    return (False, None)
+            return (True, "skipping because remote is not listed")
+            return (True, "skipping because remote is not listed")
+    return skipTestIfFn(is_remote_unlisted)
 
 def skipIfRemoteDueToDeadlock(func):
     """Decorate the item to skip tests if testing remotely due to the test deadlocking."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@skipIfRemote can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(*args, **kwargs):
-        from unittest2 import case
-        if lldb.remote_platform:
-            self = args[0]
-            self.skipTest("skip on remote platform (deadlocks)")
-        else:
-            func(*args, **kwargs)
-    return wrapper
+    def is_remote():
+        return (lldb.remote_platform, "skip on remote platform (deadlocks)")
+    return skipTestIfFn(is_remote)
 
 def skipIfNoSBHeaders(func):
     """Decorate the item to mark tests that should be skipped when LLDB is built with no SB API headers."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@skipIfNoSBHeaders can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(*args, **kwargs):
-        from unittest2 import case
-        self = args[0]
+    def are_sb_headers_missing():
         if sys.platform.startswith("darwin"):
             header = os.path.join(os.environ["LLDB_LIB_DIR"], 'LLDB.framework', 'Versions','Current','Headers','LLDB.h')
         else:
             header = os.path.join(os.environ["LLDB_SRC"], "include", "lldb", "API", "LLDB.h")
         platform = sys.platform
         if not os.path.exists(header):
-            self.skipTest("skip because LLDB.h header not found")
+            return (True, "skip because LLDB.h header not found")
         else:
-            func(*args, **kwargs)
-    return wrapper
+            return (False, None)
+    return skipTestIfFn(are_sb_headers_missing)
 
 def skipIfiOSSimulator(func):
     """Decorate the item to skip tests that should be skipped on the iOS Simulator."""
-    return unittest2.skipIf(configuration.lldb_platform_name == 'ios-simulator', 'skip on the iOS Simulator')(func)
+    def is_ios_simulator():
+        return (configuration.lldb_platform_name == 'ios-simulator', "skip on the iOS Simulator")
+    return skipTestIfFn(is_ios_simulator)
 
 def skipIfFreeBSD(func):
     """Decorate the item to skip tests that should be skipped on FreeBSD."""
     return skipIfPlatform(["freebsd"])(func)
 
 def skipIfNetBSD(func):
     """Decorate the item to skip tests that should be skipped on NetBSD."""
     return skipIfPlatform(["netbsd"])(func)
def skipUnlessWindows(func):
     return skipUnlessPlatform(["windows"])(func)
 
 def skipUnlessDarwin(func):
     """Decorate the item to skip tests that should be skipped on any non Darwin platform."""
     return skipUnlessPlatform(getDarwinOSTriples())(func)
 
 def skipUnlessGoInstalled(func):
     """Decorate the item to skip tests when no Go compiler is available."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@skipIfGcc can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(*args, **kwargs):
-        from unittest2 import case
-        self = args[0]
+    def is_go_missing(self):
         compiler = self.getGoCompilerVersion()
         if not compiler:
-            self.skipTest("skipping because go compiler not found")
-        else:
-            # Ensure the version is the minimum version supported by
-            # the LLDB go support.
-            match_version = re.search(r"(\d+\.\d+(\.\d+)?)", compiler)
-            if not match_version:
-                # Couldn't determine version.
-                self.skipTest(
-                    "skipping because go version could not be parsed "
-                    "out of {}".format(compiler))
-            else:
-                from distutils.version import StrictVersion
-                min_strict_version = StrictVersion("1.4.0")
-                compiler_strict_version = StrictVersion(match_version.group(1))
-                if compiler_strict_version < min_strict_version:
-                    self.skipTest(
-                        "skipping because available go version ({}) does "
-                        "not meet minimum required go version ({})".format(
-                            compiler_strict_version,
-                            min_strict_version))
-            func(*args, **kwargs)
-    return wrapper
+            return (True, "skipping because go compiler not found")
+            return (True, "skipping because go version could not be parsed out of {}".format(compiler))
+                return (True, "skipping because available version ({}) does not meet minimum required version ({})"
+                               .format(compiler_strict_version, min_strict_version))
+        return (False, None)
+    return skipTestIfFn(is_go_missing)
 
 def getPlatform():
     """Returns the target platform which the tests are running on."""
     platform = lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
     if platform.startswith('freebsd'):
         platform = 'freebsd'
     elif platform.startswith('netbsd'):
         platform = 'netbsd'
else:
         return sys.platform
 
 def platformIsDarwin():
     """Returns true if the OS triple for the selected platform is any valid apple OS"""
     return getPlatform() in getDarwinOSTriples()
 
 def skipIfHostIncompatibleWithRemote(func):
     """Decorate the item to skip tests if binaries built on this host are incompatible."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@skipIfHostIncompatibleWithRemote can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(*args, **kwargs):
-        from unittest2 import case
-        self = args[0]
+    def is_host_incompatible_with_remote(self):
         host_arch = self.getLldbArchitecture()
         host_platform = getHostPlatform()
         target_arch = self.getArchitecture()
         target_platform = 'darwin' if self.platformIsDarwin() else self.getPlatform()
         if not (target_arch == 'x86_64' and host_arch == 'i386') and host_arch != target_arch:
-            self.skipTest("skipping because target %s is not compatible with host architecture %s" % (target_arch, host_arch))
+            return (True, "skipping because target %s is not compatible with host architecture %s" % (target_arch, host_arch))
         elif target_platform != host_platform:
-            self.skipTest("skipping because target is %s but host is %s" % (target_platform, host_platform))
-        else:
-            func(*args, **kwargs)
-    return wrapper
+            return (True, "skipping because target is %s but host is %s" % (target_platform, host_platform))
+        return (False, None)
+    return skipTestIfFn(is_host_incompatible_with_remote)
 
 def skipIfHostPlatform(oslist):
     """Decorate the item to skip tests if running on one of the listed host platforms."""
-    return unittest2.skipIf(getHostPlatform() in oslist,
-                            "skip on %s" % (", ".join(oslist)))
+    return skipIf(hostoslist=oslist)
 
 def skipUnlessHostPlatform(oslist):
     """Decorate the item to skip tests unless running on one of the listed host platforms."""
-    return unittest2.skipUnless(getHostPlatform() in oslist,
-                                "requires on of %s" % (", ".join(oslist)))
+    return skipIf(hostoslist=not_in(oslist))
 
 def skipUnlessArch(archs):
     """Decorate the item to skip tests unless running on one of the listed architectures."""
-    def myImpl(func):
-        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-            raise Exception("@skipUnlessArch can only be used to decorate a test method")
-
-        @wraps(func)
-        def wrapper(*args, **kwargs):
-            self = args[0]
-            if not matchArchitectures(archs, self.getArchitecture()):
-                self.skipTest("skipping for architecture %s" % (self.getArchitecture())) 
-            else:
-                func(*args, **kwargs)
-        return wrapper
-
-    return myImpl
+    return skipIf(archs=not_in(archs))
 
 def skipIfPlatform(oslist):
     """Decorate the item to skip tests if running on one of the listed platforms."""
-    return unittest2.skipIf(getPlatform() in oslist,
-                            "skip on %s" % (", ".join(oslist)))
+    return skipIf(oslist=oslist)
 
 def skipUnlessPlatform(oslist):
     """Decorate the item to skip tests unless running on one of the listed platforms."""
-    return unittest2.skipUnless(getPlatform() in oslist,
-                                "requires on of %s" % (", ".join(oslist)))
+    return skipIf(oslist=not_in(oslist))
 
 # provide a function to skip on defined oslist, compiler version, and archs
 # if none is specified for any argument, that argument won't be checked and thus means for all
 # for example,
 # @skipIf, skip for all platform/compiler/arch,
 # @skipIf(compiler='gcc'), skip for gcc on all platform/architecture
 # @skipIf(bugnumber, ["linux"], "gcc", ['>=', '4.9'], ['i386']), skip for gcc>=4.9 on linux with i386
 

 
 def skipIfGcc(func):
     """Decorate the item to skip tests that should be skipped if building with gcc ."""
     if isinstance(func, type) and issubclass(func, unittest2.TestCase):
         raise Exception("@skipIfGcc can only be used to decorate a test method")
     @wraps(func)
     def wrapper(*args, **kwargs):
         from unittest2 import case
         self = args[0]
         compiler = self.getCompiler()
         if "gcc" in compiler:
             self.skipTest("skipping because gcc is the test compiler")
         else:
             func(*args, **kwargs)
     return wrapper
 
 def skipIfIcc(func):

packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py

def Hc_then_Csignal_signals_correct_thread(self, segfault_signo):
             print_thread_ids[print_thread_id] = 1
 
             # Ensure post signal-handle thread id matches the thread that initially raised the SIGSEGV.
             post_handle_thread_id = context.get("post_handle_thread_id")
             self.assertIsNotNone(post_handle_thread_id)
             post_handle_thread_id = int(post_handle_thread_id, 16)
             self.assertEqual(post_handle_thread_id, print_thread_id)
 
-    @unittest2.expectedFailure()
+    @expectedFailureAll
     @debugserver_test
     def test_Hc_then_Csignal_signals_correct_thread_launch_debugserver(self):
         self.init_debugserver_test()
         self.build()
         self.set_inferior_startup_launch()
         # Darwin debugserver translates some signals like SIGSEGV into some gdb expectations about fixed signal numbers.
         self.Hc_then_Csignal_signals_correct_thread(self.TARGET_EXC_BAD_ACCESS)