[lldb][test] Apply @expectedFailureAll/@skipIf early for debug_info tests

[rupprecht]

The @expectedFailureAll and @skipIf decorators will mark the test case as xfail/skip if all conditions passed in match, including debug_info.

• If debug_info is not one of the matching conditions, we can immediately evaluate the check and decide if it should be decorated.
• If debug_info is present as a match condition, we need to defer whether or not to decorate until when the LLDBTestCaseFactory metaclass expands the test case into its potential variants. This is still early enough that the standard unittest framework will recognize the test as xfail/skip by the time the test actually runs.

TestDecorators exhibits the edge cases more thoroughly. With the exception of @expectedFailureIf (added by this commit), all those test cases pass prior to this commit.

This is a followup to 212a60e.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-lldb

Author: Jordan Rupprecht (rupprecht)

Changes

The @expectedFailureAll and @skipIf decorators will mark the test case as xfail/skip if all conditions passed in match, including debug_info.

• If debug_info is not one of the matching conditions, we can immediately evaluate the check and decide if it should be decorated.
• If debug_info is present as a match condition, we need to defer whether or not to decorate until when the LLDBTestCaseFactory metaclass expands the test case into its potential variants. This is still early enough that the standard unittest framework will recognize the test as xfail/skip by the time the test actually runs.

TestDecorators exhibits the edge cases more thoroughly. With the exception of @<!-- -->expectedFailureIf (added by this commit), all those test cases pass prior to this commit.

This is a followup to 212a60e.

---

Full diff: https://github.com/llvm/llvm-project/pull/73067.diff

3 Files Affected:

• (modified) lldb/packages/Python/lldbsuite/test/decorators.py (+50-3)
• (modified) lldb/packages/Python/lldbsuite/test/lldbtest.py (+20)
• (modified) lldb/test/API/test_utils/TestDecorators.py (+107-3)

diff --git a/lldb/packages/Python/lldbsuite/test/decorators.py b/lldb/packages/Python/lldbsuite/test/decorators.py
index bb06a5ee20f2532..2398892b9e14814 100644
--- a/lldb/packages/Python/lldbsuite/test/decorators.py
+++ b/lldb/packages/Python/lldbsuite/test/decorators.py
@@ -117,6 +117,21 @@ def expectedFailure(func):
     return unittest2.expectedFailure(func)
 
 
+def expectedFailureIf(condition, 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")
+
+        if condition:
+            return expectedFailure(func)
+        return func
+
+    if callable(bugnumber):
+        return expectedFailure_impl(bugnumber)
+    else:
+        return expectedFailure_impl
+
+
 def expectedFailureIfFn(expected_fn, bugnumber=None):
     def expectedFailure_impl(func):
         if isinstance(func, type) and issubclass(func, unittest2.TestCase):
@@ -178,6 +193,34 @@ def wrapper(*args, **kwargs):
         return skipTestIfFn_impl
 
 
+def _xfailForDebugInfo(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")
+
+        func.__xfail_for_debug_info_cat_fn__ = expected_fn
+        return func
+
+    if callable(bugnumber):
+        return expectedFailure_impl(bugnumber)
+    else:
+        return expectedFailure_impl
+
+
+def _skipForDebugInfo(expected_fn, bugnumber=None):
+    def skipImpl(func):
+        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
+            raise Exception("Decorator can only be used to decorate a test method")
+
+        func.__skip_for_debug_info_cat_fn__ = expected_fn
+        return func
+
+    if callable(bugnumber):
+        return skipImpl(bugnumber)
+    else:
+        return skipImpl
+
+
 def _decorateTest(
     mode,
     bugnumber=None,
@@ -195,7 +238,7 @@ def _decorateTest(
     dwarf_version=None,
     setting=None,
 ):
-    def fn(self):
+    def fn(actual_debug_info=None):
         skip_for_os = _match_decorator_property(
             lldbplatform.translate(oslist), lldbplatformutil.getPlatform()
         )
@@ -208,7 +251,7 @@ def fn(self):
         skip_for_arch = _match_decorator_property(
             archs, lldbplatformutil.getArchitecture()
         )
-        skip_for_debug_info = _match_decorator_property(debug_info, self.getDebugInfo())
+        skip_for_debug_info = _match_decorator_property(debug_info, actual_debug_info)
         skip_for_triple = _match_decorator_property(
             triple, lldb.selected_platform.GetTriple()
         )
@@ -283,9 +326,13 @@ def fn(self):
         return reason_str
 
     if mode == DecorateMode.Skip:
+        if debug_info:
+            return _skipForDebugInfo(fn, bugnumber)
         return skipTestIfFn(fn, bugnumber)
     elif mode == DecorateMode.Xfail:
-        return expectedFailureIfFn(fn, bugnumber)
+        if debug_info:
+            return _xfailForDebugInfo(fn, bugnumber)
+        return expectedFailureIf(fn(), bugnumber)
     else:
         return None
 
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index dc4e322c675dc9d..872866655093d21 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -1667,6 +1667,11 @@ def __new__(cls, name, bases, attrs):
         if original_testcase.NO_DEBUG_INFO_TESTCASE:
             return original_testcase
 
+        # Default implementation for skip/xfail reason based on the debug category,
+        # where "None" means to run the test as usual.
+        def no_reason(_):
+            return None
+
         newattrs = {}
         for attrname, attrvalue in attrs.items():
             if attrname.startswith("test") and not getattr(
@@ -1688,6 +1693,12 @@ def __new__(cls, name, bases, attrs):
                         if can_replicate
                     ]
 
+                xfail_for_debug_info_cat_fn = getattr(
+                    attrvalue, "__xfail_for_debug_info_cat_fn__", no_reason
+                )
+                skip_for_debug_info_cat_fn = getattr(
+                    attrvalue, "__skip_for_debug_info_cat_fn__", no_reason
+                )
                 for cat in categories:
 
                     @decorators.add_test_categories([cat])
@@ -1698,6 +1709,15 @@ def test_method(self, attrvalue=attrvalue):
                     method_name = attrname + "_" + cat
                     test_method.__name__ = method_name
                     test_method.debug_info = cat
+
+                    xfail_reason = xfail_for_debug_info_cat_fn(cat)
+                    if xfail_reason:
+                        test_method = unittest2.expectedFailure(xfail_reason)(test_method)
+
+                    skip_reason = skip_for_debug_info_cat_fn(cat)
+                    if skip_reason:
+                        test_method = unittest2.skip(skip_reason)(test_method)
+
                     newattrs[method_name] = test_method
 
             else:
diff --git a/lldb/test/API/test_utils/TestDecorators.py b/lldb/test/API/test_utils/TestDecorators.py
index 97d144b6d44412b..eb09db69de34975 100644
--- a/lldb/test/API/test_utils/TestDecorators.py
+++ b/lldb/test/API/test_utils/TestDecorators.py
@@ -1,11 +1,115 @@
-from lldbsuite.test.lldbtest import Base
+import re
+
+from lldbsuite.test.lldbtest import TestBase
 from lldbsuite.test.decorators import *
 
 
-class TestDecorators(Base):
+def expectedFailureDwarf(bugnumber=None):
+    return expectedFailureAll(bugnumber, debug_info="dwarf")
+
+
+class TestDecoratorsNoDebugInfoClass(TestBase):
     NO_DEBUG_INFO_TESTCASE = True
 
     @expectedFailureAll(debug_info="dwarf")
-    def test_decorator_skip_no_debug_info(self):
+    def test_decorator_xfail(self):
         """Test that specifying a debug info category works for a NO_DEBUG_INFO_TESTCASE"""
+
+    @expectedFailureDwarf
+    def test_decorator_xfail_bare_decorator(self):
+        """Same as test_decorator_xfail, but with a custom decorator w/ a bare syntax"""
+
+    @expectedFailureDwarf()
+    def test_decorator_xfail_decorator_empty_args(self):
+        """Same as test_decorator_xfail, but with a custom decorator w/ no args"""
+
+    @add_test_categories(["dwarf"])
+    def test_add_test_categories(self):
+        # Note: the "dwarf" test category is ignored, because we don't generate any debug info test variants
+        self.assertIsNone(self.getDebugInfo())
+
+    @expectedFailureAll
+    def test_xfail_regexp(self):
+        """Test that expectedFailureAll can be empty (but please just use expectedFailure)"""
+        self.fail()
+
+    @expectedFailureAll(compiler=re.compile(".*"))
+    def test_xfail_regexp(self):
+        """Test that xfail can take a regex as a matcher"""
+        self.fail()
+
+    @expectedFailureAll(compiler=no_match(re.compile(".*")))
+    def test_xfail_no_match(self):
+        """Test that xfail can take a no_match matcher"""
+        pass
+
+    @expectedFailureIf(condition=True)
+    def test_xfail_condition_true(self):
+        self.fail()
+
+    @expectedFailureIf(condition=False)
+    def test_xfail_condition_false(self):
+        pass
+
+
+class TestDecorators(TestBase):
+    @expectedFailureAll(debug_info="dwarf")
+    def test_decorator_xfail(self):
+        """Test that expectedFailureAll fails for the debug_info variant"""
+        if self.getDebugInfo() == "dwarf":
+            self.fail()
+
+    @skipIf(debug_info="dwarf")
+    def test_decorator_skip(self):
+        """Test that skipIf skips the debug_info variant"""
+        self.assertNotEqual(self.getDebugInfo(), "dwarf")
+
+    @expectedFailureDwarf
+    def test_decorator_xfail2(self):
+        """Same as test_decorator_xfail, but with a custom decorator w/ a bare syntax"""
+        if self.getDebugInfo() == "dwarf":
+            self.fail()
+
+    @expectedFailureDwarf()
+    def test_decorator_xfail3(self):
+        """Same as test_decorator_xfail, but with a custom decorator w/ no args"""
+        if self.getDebugInfo() == "dwarf":
+            self.fail()
+
+    @add_test_categories(["dwarf"])
+    def test_add_test_categories(self):
+        """Test that add_test_categories limits the kinds of debug info test variants"""
+        self.assertEqual(self.getDebugInfo(), "dwarf")
+
+    @expectedFailureAll(compiler="fake", debug_info="dwarf")
+    def test_decorator_xfail_all(self):
+        """Test that expectedFailureAll requires all conditions to match to be xfail"""
+
+    @skipIf(compiler="fake", debug_info="dwarf")
+    def test_decorator_skip2(self):
+        """Test that expectedFailureAll fails for the debug_info variant"""
+        # Note: the following assertion would fail, if this were not skipped:
+        # self.assertNotEqual(self.getDebugInfo(), "dwarf")
+
+    @expectedFailureAll
+    def test_xfail_regexp(self):
+        """Test that xfail can be empty"""
+        self.fail()
+
+    @expectedFailureAll(compiler=re.compile(".*"))
+    def test_xfail_regexp(self):
+        """Test that xfail can take a regex as a matcher"""
+        self.fail()
+
+    @expectedFailureAll(compiler=no_match(re.compile(".*")))
+    def test_xfail_no_match(self):
+        """Test that xfail can take a no_match matcher"""
+        pass
+
+    @expectedFailureIf(condition=True)
+    def test_xfail_condition_true(self):
+        self.fail()
+
+    @expectedFailureIf(condition=False)
+    def test_xfail_condition_false(self):
         pass

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[rupprecht]

• This is still early enough that the standard unittest framework will recognize the test as xfail/skip by the time the test actually runs.

I did a little bit more research, turns out this is not a difference between unittest and unittest2, but rather just something we have in our ancient, vendored copy of unittest2.

Our copy of unittest2 originates from 2010 in 7325883, and has never undergone a version upgrade AFAICT. Two commits, both applied Mar 20, 2013, implement some features for subTest in both unittest and unittest2 for https://bugs.python.org/issue16997:

• python/cpython@c9b3ef2
• https://hg.python.org/unittest2/rev/0daffa9ee7c1

The important part of that diff is it stops raising _ExpectedFailure and starts just setting __unittest_expecting_failure__ on the test method. The full discussion on that is https://bugs.python.org/issue16997, especially starting around msg183450

[rupprecht]

ping :)

[adrian-prantl]

Is this a performance optimization or a function al change?

[rupprecht]

Is this a performance optimization or a function al change?

Neither. This should only affect tests, and the set of tests we skip/mark as xfail should not change, we just generally know about it earlier in the test setup.

[adrian-prantl]

SGTM, maybe wait one more day for @JDevlieghere to chime in.