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 ↗

(On Diff #46449)

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 ↗

(On Diff #46449)

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 ↗

(On Diff #46449)

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 ↗

(On Diff #46599)

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 ↗

(On Diff #46599)

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 ↗

(On Diff #46599)

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 ↗

(On Diff #46599)

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 ↗

(On Diff #46599)

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 ↗

(On Diff #46599)

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 ↗

(On Diff #46599)

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 ↗

(On Diff #46599)

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

lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py

 
 def builder_module():
     if sys.platform.startswith("freebsd"):
         return __import__("builder_freebsd")
     if sys.platform.startswith("netbsd"):
         return __import__("builder_netbsd")
     return __import__("builder_" + sys.platform)
 
+def does_function_require_self(func):
+    import inspect
+    func_argc = len(inspect.getargspec(func).args)
+    if func_argc == 0 or (getattr(func,'im_self',None) is not None) or (hasattr(func, '__self__')):
+        return False
+    else:
+        return True
+
 def run_adb_command(cmd, device_id):
     device_id_args = []
     if device_id:
         device_id_args = ["-s", device_id]
     full_cmd = ["adb"] + device_id_args + cmd
     p = Popen(full_cmd, stdout=PIPE, stderr=PIPE)
     stdout, stderr = p.communicate()
     return p.returncode, stdout, stderr
def impl(func):
             cat.extend(func.categories)
         func.categories = cat
         return func
 
     return impl
 
 def benchmarks_test(func):
     """Decorate the item as a benchmarks test."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@benchmarks_test can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(self, *args, **kwargs):
-        self.skipTest("benchmarks test")
-        return func(self, *args, **kwargs)
+    def should_skip_benchmarks_test():
+        return (True, "benchmarks test")
 
     # Mark this function as such to separate them from the regular tests.
-    wrapper.__benchmarks_test__ = True
-    return wrapper
+    result = skipTestIfFn(should_skip_benchmarks_test)(func)
+    result.__benchmarks_test__ = True
+    return result
 
 def no_debug_info_test(func):
     """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_test():
+        return (configuration.dont_do_debugserver_test, "debugserver tests")
+    return skipTestIfFn(should_skip_debugserver_test)(func)
 
 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)(func)
 
 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)(func)
 
 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

 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)(func)
 
 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 (False, None)
+    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)(func)
 
 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")
-        else:
-            func(*args, **kwargs)
-    return wrapper
+            return (True, "skip because LLDB.h header not found")
+
+        return (False, None)
+    return skipTestIfFn(are_sb_headers_missing)(func)
 
 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)(func)
 
 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)(func)
 
 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)(func)
 
 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."""
+    # This decorator cannot be ported to `skipIf` yet because it is uused with regular
+    # expressions, which the common matcher does not yet support.
     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
 
 def skipIfPlatform(oslist):
     """Decorate the item to skip tests if running on one of the listed platforms."""
+    # This decorator cannot be ported to `skipIf` yet because it is used on entire
+    # classes, which `skipIf` explicitly forbids.
     return unittest2.skipIf(getPlatform() in oslist,
                             "skip on %s" % (", ".join(oslist)))
 
 def skipUnlessPlatform(oslist):
     """Decorate the item to skip tests unless running on one of the listed platforms."""
+    # This decorator cannot be ported to `skipIf` yet because it is used on entire
+    # classes, which `skipIf` explicitly forbids.
     return unittest2.skipUnless(getPlatform() in oslist,
                                 "requires on of %s" % (", ".join(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

 def skipIfDwarf(bugnumber=None):
     return skipIfDebugInfo(bugnumber, ["dwarf"])
 
 def skipIfDsym(bugnumber=None):
     return skipIfDebugInfo(bugnumber, ["dsym"])
 
 def skipTestIfFn(expected_fn, bugnumber=None):
     def skipTestIfFn_impl(func):
+        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
+            raise Exception("@skipTestIfFn can only be used to decorate a test method")
+
         @wraps(func)
         def wrapper(*args, **kwargs):
             from unittest2 import case
             self = args[0]
+            if does_function_require_self(expected_fn):
                 skip, reason = expected_fn(self)
+            else:
+                skip, reason = expected_fn()
             if skip:
                self.skipTest(reason)
             else:
                 func(*args, **kwargs)
         return wrapper
 
     # Some decorators can be called both with no arguments (e.g. @expectedFailureWindows)
     # or with arguments (e.g. @expectedFailureWindows(compilers=['gcc'])).  When called
     # the first way, the first argument will be the actual function because decorators are
     # weird like that.  So this is basically a check that says "how was the decorator used"
     if six.callable(bugnumber):
         return skipTestIfFn_impl(bugnumber)
     else:
         return skipTestIfFn_impl
 
 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
+    return skipIf(compiler="gcc")(func)
 
 def skipIfIcc(func):
     """Decorate the item to skip tests that should be skipped if building with icc ."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@skipIfIcc 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 "icc" in compiler:
-            self.skipTest("skipping because icc is the test compiler")
-        else:
-            func(*args, **kwargs)
-    return wrapper
+    return skipIf(compiler="icc")(func)
 
 def skipIfi386(func):
     """Decorate the item to skip tests that should be skipped if building 32-bit."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@skipIfi386 can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(*args, **kwargs):
-        from unittest2 import case
-        self = args[0]
-        if "i386" == self.getArchitecture():
-            self.skipTest("skipping because i386 is not a supported architecture")
-        else:
-            func(*args, **kwargs)
-    return wrapper
+    return skipIf(archs="i386")(func)
 
 def skipIfTargetAndroid(api_levels=None, archs=None):
     """Decorator to skip tests when the target is Android.
 
     Arguments:
         api_levels - The API levels for which the test should be skipped. If
             it is None, then the test will be skipped for all API levels.
         arch - A sequence of architecture names specifying the architectures
             for which a test is skipped. None means all architectures.
     """
-    def myImpl(func):
-        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-            raise Exception("@skipIfTargetAndroid can only be used to "
-                            "decorate a test method")
-        @wraps(func)
-        def wrapper(*args, **kwargs):
-            from unittest2 import case
-            self = args[0]
-            skip_for_android, reason = matchAndroid(api_levels, archs)(self)
-            if skip_for_android:
-                self.skipTest("skiped on Android target with API %d and architecture %s.  %s" %
-                        (android_device_api(), self.getArchitecture(), reason))
-            func(*args, **kwargs)
-        return wrapper
-    return myImpl
+    def is_target_android(self):
+        return matchAndroid(api_levels, archs)(self)
+    return skipTestIfFn(is_target_android)
 
 def skipUnlessCompilerRt(func):
     """Decorate the item to skip tests if testing remotely."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@skipUnless can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(*args, **kwargs):
-        from unittest2 import case
-        import os.path
+    def is_compiler_rt_missing():
         compilerRtPath = os.path.join(os.path.dirname(__file__), "..", "..", "..", "..", "llvm","projects","compiler-rt")
-        print(compilerRtPath)
-        if not os.path.exists(compilerRtPath):
-            self = args[0]
-            self.skipTest("skip if compiler-rt not found")
-        else:
-            func(*args, **kwargs)
-    return wrapper
+        return (not os.path.exists(compilerRtPath), "compiler-rt not found")
+    return skipTestIfFn(is_compiler_rt_missing)(func)
 
 class _PlatformContext(object):
     """Value object class which contains platform-specific options."""
 
     def __init__(self, shlib_environment_var, shlib_prefix, shlib_extension):
         self.shlib_environment_var = shlib_environment_var
         self.shlib_prefix = shlib_prefix
         self.shlib_extension = shlib_extension
def tearDown(self):
         #traceback.print_stack()
 
         self.deletePexpectChild()
 
         # Check and run any hook functions.
         for hook in reversed(self.hooks):
             with recording(self, traceAlways) as sbuf:
                 print("Executing tearDown hook:", getsource_if_available(hook), file=sbuf)
-            import inspect
-            hook_argc = len(inspect.getargspec(hook).args)
-            if hook_argc == 0 or (getattr(hook,'im_self',None) is not None) or (hasattr(hook, '__self__')):
-                hook()
-            elif hook_argc == 1:
+            if does_function_require_self(hook):
                 hook(self)
             else:
                 hook() # try the plain call and hope it works
 
         del self.hooks
 
         # Perform registered teardown cleanup.
         if doCleanup and self.doTearDownCleanup: