A number of improvements to decorator conditionals

[zturner]

Summary

The goal here is to try to improve consistency across the variety of decorators we support. This patch makes the following changes:

• Change the not_in function to be a class object called no_match. This indicates that it can be used for more than just checking list membership, and that instead it negates whatever condition is used to normally match.
• Change the name of _check_list_or_lambda to match_decorator_property. This indicates that it could be more than a list or a lambda.
• Always use a regex match when matching strings in match_decorator_property. This allows any string on any decoratory property to automatically support regex matching.
• Also support the use of compiled regexes on decorator properties.
• Fix a bug in the compiler check used by _decorateTest. The two arguments were reversed, so that check was not matching when it should have matched.
• Change one test that uses skipUnlessArch to use skipIf. Now that skipIf supports regex matching on the architecture property, this works, and as a followup skipUnlessArch will be removed entirely.

Note that you won't be able to apply this patch unless you apply it on top of my previous review D16936.

[zturner]

Minor change that allows any iterable to be used (not just lists), and also recurses through the _match_decorator_property function for each item in the iterable, instead of just doing a simple list membership. Combined with the regex change and the no_match change from function to object, this allows complex specifications such as archs=['mips.*', 'i[0-9]86', no_match('x86_x64)'] (not that that particular condition makes sense, but might as well support it).

[tfiala]

I'll have a look at this after I get through D16936.

Concept sounds great.

[labath]

Looks good, after applying one fix.

packages/Python/lldbsuite/test/decorators.py

150

The actual value should be simply lldb.remote_platform is not None. When remote_platform is None, we are local, when it is not None, we are remote.

Btw, since this flag can only have three values (None, True, False), the _match_decorator property dance is not really necessary. Although, I guess it won't hurt either...

[tfiala]

LGTM.

Regarding the section Pavel was looking at, on the Apple side we have a simulator target that is not technically remote, but isn't really local either. I'll have to look at that if/when we actually want to mark a simulator test as failed.

packages/Python/lldbsuite/test/decorators.py

 class DecorateMode:
     Skip, Xfail = range(2)
 
+    
+# You can use no_match to reverse the test of the conditional that is used to match keyword
+# arguments in the skip / xfail decorators.  If oslist=["windows", "linux"] skips windows
+# and linux, oslist=no_match(["windows", "linux"]) skips *unless* windows or linux.
+class no_match:
+    def __init__(self, item):
+        self.item = item
 
 def _check_expected_version(comparison, expected, actual):
     def fn_leq(x,y): return x <= y

 
     return op_lookup[comparison](LooseVersion(actual_str), LooseVersion(expected_str))
 
-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:
+def _match_decorator_property(expected, actual):
+    if actual is None or expected is None:
+        return True
+
+    if isinstance(expected, no_match):
+        return not _match_decorator_property(expected.item, actual)
+    elif isinstance(expected, list):
+        for item in expected:
+            if actual in item:
                 return True
         return False
-    elif isinstance(list_or_lambda, str):
-        return value is None or value in list_or_lambda
+    elif isinstance(expected, (str, re._pattern_type)):
+        return re.search(expected, actual) is not None
     else:
-        return list_or_lambda is None or value is None or list_or_lambda == value
+        return expected == actual
 
 def expectedFailure(expected_fn, bugnumber=None):
     def expectedFailure_impl(func):

                  swig_version=None, py_version=None,
                  remote=None):
     def fn(self):
-        skip_for_os = _check_list_or_lambda(oslist, self.getPlatform())
-        skip_for_hostos = _check_list_or_lambda(hostoslist, lldbplatformutil.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_os = _match_decorator_property(oslist, self.getPlatform())
+        skip_for_hostos = _match_decorator_property(hostoslist, lldbplatformutil.getHostPlatform())
+        skip_for_compiler = _match_decorator_property(compiler, self.getCompiler()) and self.expectedCompilerVersion(compiler_version)
+        skip_for_arch = _match_decorator_property(archs, self.getArchitecture())
+        skip_for_debug_info = _match_decorator_property(debug_info, self.debug_info)
+        skip_for_triple = _match_decorator_property(triple, lldb.DBG.GetSelectedPlatform().GetTriple())
+        skip_for_remote = _match_decorator_property(remote, remote == (lldb.remote_platform is not None))
+
         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

         return "Not ready for remote testsuite" if lldb.remote_platform else None
     return skipTestIfFn(is_remote)(func)
 
-# 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 _match_architectures(archs, actual_arch):
     retype = type(re.compile('hello, world'))
     list_passes = isinstance(archs, list) and actual_arch in archs

 
 def skipUnlessHostPlatform(oslist):
     """Decorate the item to skip tests unless running on one of the listed host platforms."""
-    return skipIf(hostoslist=not_in(oslist))
+    return skipIf(hostoslist=no_match(oslist))
 
 def skipUnlessArch(archs):
     """Decorate the item to skip tests unless running on one of the listed architectures."""

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

 
     mydir = TestBase.compute_mydir(__file__)
 
-    @skipUnlessArch(archs=re.compile('mips*'))
+    @skipIf(archs=no_match(re.compile('mips*')))
     def test(self):
         self.build()
         exe = os.path.join(os.getcwd(), "a.out")

packages/Python/lldbsuite/test/functionalities/breakpoint/debugbreak/TestDebugBreak.py

 
     mydir = TestBase.compute_mydir(__file__)
 
-    @skipIf(archs=not_in(["i386", "i686"]))
+    @skipIf(archs=no_match(["i386", "i686"]))
     @no_debug_info_test
     def test_asm_int_3(self):
         """Test that intrinsics like `__debugbreak();` and `asm {"int3"}` are treated like breakpoints."""

packages/Python/lldbsuite/test/lang/cpp/limit-debug-info/TestWithLimitDebugInfo.py

 
     mydir = TestBase.compute_mydir(__file__)
 
-    @skipIf(debug_info=not_in(["dwarf"]))
+    @skipIf(debug_info=no_match(["dwarf"]))
     def test_limit_debug_info(self):
         self.build()