zturner created this revision.
zturner added reviewers: tfiala, labath.
zturner added a subscriber: lldb-commits.
Herald added subscribers: srhines, danalbert, tberghammer.

This is going to be part of a larger effort to clean up some of these 
decorators as the code is getting really unwieldy and difficult to understand 
and maintain.  For now, this patch:

a) Changes the `skipIf` decorator to be called `decorateTest` and gives it a 
`mode` parameter that says whether we're skipping or xfailing.  This allows the 
skip and xfail decorators to take the same set of arguments and use the same 
logic for checking the condition so that we only have 1 large and ugly function 
instead of 2 large and ugly functions that diverge from each other.

b) Update `expectedFailureAll` and `skipIf` to call `decorateTest`, so that all 
existing code using those decorators go through the common code path, and so 
that `skipIf` and `expectedFailureAll` now support the same set of arguments.

c) Change the way the skip / xfail "reason" is computed to be more friendly.

d) Delete this weird code that says `if six.callable(bugnumber)` everywhere 
since it appears to be an invalid assumption about what a bugnumber is (i.e. 
always a string, never a callable).

Ran the test suite before and after my CL, and the summary statistics at the 
end are identical in both cases.

In future patches I intend to:

a) Move all the decorators to another file somewhere so they are all isolated 
and not lost in a sea of 10,000 lines of other code.

b) Remove some of them and consolidate to using the "master" decorators that 
can are essentially a superset of some of the existing specialized decorators.

c) Add a command line option that causes the test suite to treat skips as 
xfails.


http://reviews.llvm.org/D16615

Files:
  
packages/Python/lldbsuite/test/functionalities/jitloader_gdb/TestJITLoaderGDB.py
  packages/Python/lldbsuite/test/lldbtest.py

Index: packages/Python/lldbsuite/test/lldbtest.py
===================================================================
--- packages/Python/lldbsuite/test/lldbtest.py
+++ packages/Python/lldbsuite/test/lldbtest.py
@@ -512,6 +512,9 @@
 #
 # Decorators for categorizing test cases.
 #
+class DecorateMode:
+    Skip, Xfail = range(2)
+
 from functools import wraps
 
 def add_test_categories(cat):
@@ -603,7 +606,8 @@
         def wrapper(*args, **kwargs):
             from unittest2 import case
             self = args[0]
-            if expected_fn(self):
+            xfail, reason = expected_fn(self)
+            if xfail:
                 if configuration.results_formatter_object is not None:
                     # Mark this test as expected to fail.
                     configuration.results_formatter_object.handle_event(
@@ -645,26 +649,21 @@
 # @expectedFailureAll, xfail for all platform/compiler/arch,
 # @expectedFailureAll(compiler='gcc'), xfail for gcc on all platform/architecture
 # @expectedFailureAll(bugnumber, ["linux"], "gcc", ['>=', '4.9'], ['i386']), xfail for gcc>=4.9 on linux with i386
-def expectedFailureAll(bugnumber=None, oslist=None, hostoslist=None, compiler=None, compiler_version=None, archs=None, triple=None, debug_info=None, swig_version=None, py_version=None):
-    def fn(self):
-        oslist_passes = check_list_or_lambda(oslist, self.getPlatform())
-        hostoslist_passes = check_list_or_lambda(hostoslist, getHostPlatform())
-        compiler_passes = check_list_or_lambda(self.getCompiler(), compiler) and self.expectedCompilerVersion(compiler_version)
-        arch_passes = check_list_or_lambda(archs, self.getArchitecture())
-        triple_passes = triple is None or re.match(triple, lldb.DBG.GetSelectedPlatform().GetTriple())
-        debug_info_passes = check_list_or_lambda(debug_info, self.debug_info)
-        swig_version_passes = (swig_version is None) or (not hasattr(lldb, 'swig_version')) or (check_expected_version(swig_version[0], swig_version[1], lldb.swig_version))
-        py_version_passes = (py_version is None) or check_expected_version(py_version[0], py_version[1], sys.version_info)
-
-        return (oslist_passes and
-                hostoslist_passes and
-                compiler_passes and
-                arch_passes and
-                triple_passes and
-                debug_info_passes and
-                swig_version_passes and
-                py_version_passes)
-    return expectedFailure(fn, bugnumber)
+def expectedFailureAll(bugnumber=None,
+                       oslist=None, hostoslist=None,
+                       compiler=None, compiler_version=None,
+                       archs=None, triple=None,
+                       debug_info=None,
+                       swig_version=None, py_version=None,
+                       remote=None):
+    return decorateTest(DecorateMode.Xfail,
+                        bugnumber=bugnumber,
+                        oslist=oslist, hostoslist=hostoslist,
+                        compiler=compiler, compiler_version=compiler_version,
+                        archs=archs, triple=triple,
+                        debug_info=debug_info,
+                        swig_version=swig_version, py_version=py_version,
+                        remote=remote)
 
 def expectedFailureDwarf(bugnumber=None):
     return expectedFailureAll(bugnumber=bugnumber, debug_info="dwarf")
@@ -692,9 +691,7 @@
     return expectedFailureCompiler('icc', None, bugnumber)
 
 def expectedFailureArch(arch, bugnumber=None):
-    def fn(self):
-        return arch in self.getArchitecture()
-    return expectedFailure(fn, bugnumber)
+    return decorateTest(DecorateMode.Xfail, archs=arch, bugnumber=bugnumber)
 
 def expectedFailurei386(bugnumber=None):
     return expectedFailureArch('i386', bugnumber)
@@ -703,18 +700,10 @@
     return expectedFailureArch('x86_64', bugnumber)
 
 def expectedFailureOS(oslist, bugnumber=None, compilers=None, debug_info=None, archs=None):
-    def fn(self):
-        return (self.getPlatform() in oslist and
-                self.expectedCompiler(compilers) and
-                (archs is None or self.getArchitecture() in archs) and
-                (debug_info is None or self.debug_info in debug_info))
-    return expectedFailure(fn, bugnumber)
+    return decorateTest(DecorateMode.Xfail, oslist=oslist, bugnumber=bugnumber, compiler=compilers, archs=archs, debug_info=debug_info)
 
 def expectedFailureHostOS(oslist, bugnumber=None, compilers=None):
-    def fn(self):
-        return (getHostPlatform() in oslist and
-                self.expectedCompiler(compilers))
-    return expectedFailure(fn, bugnumber)
+    return decorateTest(DecorateMode.Xfail, hostoslist=oslist, bugnumber=bugnumber)
 
 def expectedFailureDarwin(bugnumber=None, compilers=None, debug_info=None):
     # For legacy reasons, we support both "darwin" and "macosx" as OS X triples.
@@ -738,12 +727,12 @@
 def matchAndroid(api_levels=None, archs=None):
     def match(self):
         if not target_is_android():
-            return False
+            return (False, "target is not android")
         if archs is not None and self.getArchitecture() not in archs:
-            return False
+            return (False, "invalid architecture")
         if api_levels is not None and android_device_api() not in api_levels:
-            return False
-        return True
+            return (False, "invalid api level")
+        return (True, None)
     return match
 
 
@@ -1068,30 +1057,74 @@
 # @skipIf(bugnumber, ["linux"], "gcc", ['>=', '4.9'], ['i386']), skip for gcc>=4.9 on linux with i386
 
 # TODO: refactor current code, to make skipIfxxx functions to call this function
-def skipIf(bugnumber=None, oslist=None, compiler=None, compiler_version=None, archs=None, debug_info=None, swig_version=None, py_version=None, remote=None):
+def decorateTest(mode,
+                 bugnumber=None, oslist=None, hostoslist=None,
+                 compiler=None, compiler_version=None,
+                 archs=None, triple=None,
+                 debug_info=None,
+                 swig_version=None, py_version=None,
+                 remote=None):
     def fn(self):
-        oslist_passes = check_list_or_lambda(oslist, self.getPlatform())
-        compiler_passes = check_list_or_lambda(self.getCompiler(), compiler) and self.expectedCompilerVersion(compiler_version)
-        arch_passes = check_list_or_lambda(archs, self.getArchitecture())
-        debug_info_passes = check_list_or_lambda(debug_info, self.debug_info)
-        swig_version_passes = (swig_version is None) or (not hasattr(lldb, 'swig_version')) or (check_expected_version(swig_version[0], swig_version[1], lldb.swig_version))
-        py_version_passes = (py_version is None) or check_expected_version(py_version[0], py_version[1], sys.version_info)
-        remote_passes = (remote is None) or (remote == (lldb.remote_platform is not None))
-
-        return (oslist_passes and
-                compiler_passes and
-                arch_passes and
-                debug_info_passes and
-                swig_version_passes and
-                py_version_passes and
-                remote_passes)
-
-    local_vars = locals()
-    args = [x for x in inspect.getargspec(skipIf).args]
-    arg_vals = [eval(x, globals(), local_vars) for x in args]
-    args = [x for x in zip(args, arg_vals) if x[1] is not None]
-    reasons = ['%s=%s' % (x, str(y)) for (x,y) in args]
-    return skipTestIfFn(fn, bugnumber, skipReason='skipping because ' + ' && '.join(reasons))
+        skip_for_os = check_list_or_lambda(oslist, self.getPlatform())
+        skip_for_hostos = check_list_or_lambda(hostoslist, getHostPlatform())
+        skip_for_compiler = check_list_or_lambda(self.getCompiler(), compiler) and self.expectedCompilerVersion(compiler_version)
+        skip_for_arch = check_list_or_lambda(archs, self.getArchitecture())
+        skip_for_debug_info = check_list_or_lambda(debug_info, self.debug_info)
+        skip_for_triple = triple is None or re.match(triple, lldb.DBG.GetSelectedPlatform().GetTriple())
+        skip_for_swig_version = (swig_version is None) or (not hasattr(lldb, 'swig_version')) or (check_expected_version(swig_version[0], swig_version[1], lldb.swig_version))
+        skip_for_py_version = (py_version is None) or check_expected_version(py_version[0], py_version[1], sys.version_info)
+        skip_for_remote = (remote is None) or (remote == (lldb.remote_platform is not None))
+
+        # For the test to be skipped, all specified (e.g. not None) parameters must be True.
+        # An unspecified parameter means "any", so those are marked skip by default.  And we skip
+        # the final test if all conditions are True.
+        conditions = [(oslist, skip_for_os, "target o/s"),
+                      (hostoslist, skip_for_hostos, "host o/s"),
+                      (compiler, skip_for_compiler, "compiler or version"),
+                      (archs, skip_for_arch, "architecture"),
+                      (debug_info, skip_for_debug_info, "debug info format"),
+                      (triple, skip_for_triple, "target triple"),
+                      (swig_version, skip_for_swig_version, "swig version"),
+                      (py_version, skip_for_py_version, "python version"),
+                      (remote, skip_for_remote, "platform locality (remote/local)")]
+        reasons = []
+        final_skip_result = True
+        for this_condition in conditions:
+            final_skip_result = final_skip_result and this_condition[1]
+            if this_condition[0] is not None and this_condition[1]:
+                reasons.append(this_condition[2])
+        reason_str = None
+        if final_skip_result:
+            mode_str = {DecorateMode.Skip : "skipping", DecorateMode.Xfail : "xfailing"}[mode]
+            if len(reasons) > 0:
+                reason_str = ",".join(reasons)
+                reason_str = "{} due to the following parameter(s): {}".format(mode_str, reason_str)
+            else:
+                reason_str = "{} unconditionally"
+        return (final_skip_result, reason_str)
+
+    if mode == DecorateMode.Skip:
+        return skipTestIfFn(fn, bugnumber)
+    elif mode == DecorateMode.Xfail:
+        return expectedFailure(fn, bugnumber)
+    else:
+        return None
+
+def skipIf(bugnumber=None,
+           oslist=None, hostoslist=None,
+           compiler=None, compiler_version=None,
+           archs=None, triple=None,
+           debug_info=None,
+           swig_version=None, py_version=None,
+           remote=None):
+    return decorateTest(DecorateMode.Skip,
+                        bugnumber=bugnumber,
+                        oslist=oslist, hostoslist=hostoslist,
+                        compiler=compiler, compiler_version=compiler_version,
+                        archs=archs, triple=triple,
+                        debug_info=debug_info,
+                        swig_version=swig_version, py_version=py_version,
+                        remote=remote)
 
 def skipIfDebugInfo(bugnumber=None, debug_info=None):
     return skipIf(bugnumber=bugnumber, debug_info=debug_info)
@@ -1105,21 +1138,19 @@
 def skipIfDsym(bugnumber=None):
     return skipIfDebugInfo(bugnumber, ["dsym"])
 
-def skipTestIfFn(expected_fn, bugnumber=None, skipReason=None):
+def skipTestIfFn(expected_fn, bugnumber=None):
     def skipTestIfFn_impl(func):
         @wraps(func)
         def wrapper(*args, **kwargs):
             from unittest2 import case
             self = args[0]
-            if expected_fn(self):
-               self.skipTest(skipReason)
+            skip, reason = expected_fn(self)
+            if skip:
+               self.skipTest(reason)
             else:
                 func(*args, **kwargs)
         return wrapper
-    if six.callable(bugnumber):
-        return skipTestIfFn_impl(bugnumber)
-    else:
-        return skipTestIfFn_impl
+    return skipTestIfFn_impl
 
 def skipIfGcc(func):
     """Decorate the item to skip tests that should be skipped if building with gcc ."""
@@ -1182,9 +1213,10 @@
         def wrapper(*args, **kwargs):
             from unittest2 import case
             self = args[0]
-            if matchAndroid(api_levels, archs)(self):
-                self.skipTest("skiped on Android target with API %d and architecture %s" %
-                        (android_device_api(), self.getArchitecture()))
+            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
Index: packages/Python/lldbsuite/test/functionalities/jitloader_gdb/TestJITLoaderGDB.py
===================================================================
--- packages/Python/lldbsuite/test/functionalities/jitloader_gdb/TestJITLoaderGDB.py
+++ packages/Python/lldbsuite/test/functionalities/jitloader_gdb/TestJITLoaderGDB.py
@@ -16,7 +16,7 @@
 
     mydir = TestBase.compute_mydir(__file__)
 
-    @skipTestIfFn(lambda x: True, "llvm.org/pr24702", "Skipped because the test crashes the test runner")
+    @skipTestIfFn(lambda x: (True, "Skipped because the test crashes the test runner"), bugnumber="llvm.org/pr24702")
     @unittest2.expectedFailure("llvm.org/pr24702")
     def test_bogus_values(self):
         """Test that we handle inferior misusing the GDB JIT interface"""
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to