Skip to content

[lldb][test] Switch LLDB API tests from vendored unittest2 to unittest #79945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 13, 2024

Conversation

rupprecht
Copy link
Collaborator

This removes the dependency LLDB API tests have on lldb/third_party/Python/module/unittest2, and instead uses the standard one provided by Python.

This does not actually remove the vendored dep yet, nor update the docs. I'll do both those once this sticks.

Non-trivial changes to call out:

  • expected failures (i.e. "bugnumber") don't have a reason anymore, so those params were removed
  • assertItemsEqual is now called assertCountEqual
  • When a test is marked xfail, our copy of unittest2 considers failures during teardown to be OK, but modern unittest does not. See TestThreadLocal.py. (Very likely could be a real bug/leak).
  • Our copy of unittest2 was patched to print all test results, even ones that don't happen, e.g. (5 passes, 0 failures, 1 errors, 0 skipped, ...), but standard unittest prints a terser message that omits test result types that didn't happen, e.g. OK (skipped=1). Our lit integration parses this stderr and needs to be updated w/ that expectation.

I tested this w/ ninja check-lldb-abi on Linux. There's a good chance non-Linux tests have similar quirks, but I'm not able to uncover those.

@llvmbot
Copy link
Member

llvmbot commented Jan 30, 2024

@llvm/pr-subscribers-lldb

Author: Jordan Rupprecht (rupprecht)

Changes

This removes the dependency LLDB API tests have on lldb/third_party/Python/module/unittest2, and instead uses the standard one provided by Python.

This does not actually remove the vendored dep yet, nor update the docs. I'll do both those once this sticks.

Non-trivial changes to call out:

  • expected failures (i.e. "bugnumber") don't have a reason anymore, so those params were removed
  • assertItemsEqual is now called assertCountEqual
  • When a test is marked xfail, our copy of unittest2 considers failures during teardown to be OK, but modern unittest does not. See TestThreadLocal.py. (Very likely could be a real bug/leak).
  • Our copy of unittest2 was patched to print all test results, even ones that don't happen, e.g. (5 passes, 0 failures, 1 errors, 0 skipped, ...), but standard unittest prints a terser message that omits test result types that didn't happen, e.g. OK (skipped=1). Our lit integration parses this stderr and needs to be updated w/ that expectation.

I tested this w/ ninja check-lldb-abi on Linux. There's a good chance non-Linux tests have similar quirks, but I'm not able to uncover those.


Patch is 31.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/79945.diff

20 Files Affected:

  • (modified) lldb/packages/Python/lldbsuite/test/configuration.py (+2-2)
  • (modified) lldb/packages/Python/lldbsuite/test/decorators.py (+17-17)
  • (modified) lldb/packages/Python/lldbsuite/test/dotest.py (+6-6)
  • (modified) lldb/packages/Python/lldbsuite/test/lldbtest.py (+9-19)
  • (modified) lldb/packages/Python/lldbsuite/test/test_result.py (+9-9)
  • (modified) lldb/test/API/commands/expression/test/TestExprs.py (+2-2)
  • (modified) lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py (+1-1)
  • (modified) lldb/test/API/functionalities/jitloader_gdb/TestJITLoaderGDB.py (+2-2)
  • (modified) lldb/test/API/functionalities/thread/state/TestThreadStates.py (+3-3)
  • (modified) lldb/test/API/functionalities/tty/TestTerminal.py (+3-3)
  • (modified) lldb/test/API/lang/c/shared_lib/TestSharedLib.py (+2-2)
  • (modified) lldb/test/API/lang/c/shared_lib_stripped_symbols/TestSharedLibStrippedSymbols.py (+2-2)
  • (modified) lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py (+5-5)
  • (modified) lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py (+2-2)
  • (modified) lldb/test/API/lang/cpp/thread_local/TestThreadLocal.py (+5)
  • (modified) lldb/test/API/lang/objc/hidden-ivars/TestHiddenIvars.py (+2-2)
  • (modified) lldb/test/API/lldbtest.py (+34-9)
  • (modified) lldb/test/API/macosx/universal/TestUniversal.py (+4-4)
  • (modified) lldb/test/API/tools/lldb-server/libraries-svr4/TestGdbRemoteLibrariesSvr4Support.py (+1-1)
  • (modified) lldb/test/API/tools/lldb-server/test/test_lldbgdbserverutils.py (+2-2)
diff --git a/lldb/packages/Python/lldbsuite/test/configuration.py b/lldb/packages/Python/lldbsuite/test/configuration.py
index 2a4b9b3c070c7..685f491c85fe1 100644
--- a/lldb/packages/Python/lldbsuite/test/configuration.py
+++ b/lldb/packages/Python/lldbsuite/test/configuration.py
@@ -12,14 +12,14 @@
 
 
 # Third-party modules
-import unittest2
+import unittest
 
 # LLDB Modules
 import lldbsuite
 
 
 # The test suite.
-suite = unittest2.TestSuite()
+suite = unittest.TestSuite()
 
 # The list of categories we said we care about
 categories_list = None
diff --git a/lldb/packages/Python/lldbsuite/test/decorators.py b/lldb/packages/Python/lldbsuite/test/decorators.py
index 0fb146913388e..a5e1fa51cf6e6 100644
--- a/lldb/packages/Python/lldbsuite/test/decorators.py
+++ b/lldb/packages/Python/lldbsuite/test/decorators.py
@@ -11,7 +11,7 @@
 import subprocess
 
 # Third-party modules
-import unittest2
+import unittest
 
 # LLDB modules
 import lldb
@@ -115,11 +115,11 @@ def _compiler_supports(
 
 def expectedFailureIf(condition, bugnumber=None):
     def expectedFailure_impl(func):
-        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
+        if isinstance(func, type) and issubclass(func, unittest.TestCase):
             raise Exception("Decorator can only be used to decorate a test method")
 
         if condition:
-            return unittest2.expectedFailure(func)
+            return unittest.expectedFailure(func)
         return func
 
     if callable(bugnumber):
@@ -130,14 +130,14 @@ def expectedFailure_impl(func):
 
 def expectedFailureIfFn(expected_fn, bugnumber=None):
     def expectedFailure_impl(func):
-        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
+        if isinstance(func, type) and issubclass(func, unittest.TestCase):
             raise Exception("Decorator can only be used to decorate a test method")
 
         @wraps(func)
         def wrapper(*args, **kwargs):
             xfail_reason = expected_fn(*args, **kwargs)
             if xfail_reason is not None:
-                xfail_func = unittest2.expectedFailure(func)
+                xfail_func = unittest.expectedFailure(func)
                 xfail_func(*args, **kwargs)
             else:
                 func(*args, **kwargs)
@@ -157,11 +157,11 @@ def wrapper(*args, **kwargs):
 
 def skipTestIfFn(expected_fn, bugnumber=None):
     def skipTestIfFn_impl(func):
-        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
+        if isinstance(func, type) and issubclass(func, unittest.TestCase):
             reason = expected_fn()
             # The return value is the reason (or None if we don't skip), so
             # reason is used for both args.
-            return unittest2.skipIf(condition=reason, reason=reason)(func)
+            return unittest.skipIf(condition=reason, reason=reason)(func)
 
         @wraps(func)
         def wrapper(*args, **kwargs):
@@ -191,7 +191,7 @@ def wrapper(*args, **kwargs):
 
 def _xfailForDebugInfo(expected_fn, bugnumber=None):
     def expectedFailure_impl(func):
-        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
+        if isinstance(func, type) and issubclass(func, unittest.TestCase):
             raise Exception("Decorator can only be used to decorate a test method")
 
         func.__xfail_for_debug_info_cat_fn__ = expected_fn
@@ -205,7 +205,7 @@ def expectedFailure_impl(func):
 
 def _skipForDebugInfo(expected_fn, bugnumber=None):
     def skipImpl(func):
-        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
+        if isinstance(func, type) and issubclass(func, unittest.TestCase):
             raise Exception("Decorator can only be used to decorate a test method")
 
         func.__skip_for_debug_info_cat_fn__ = expected_fn
@@ -434,7 +434,7 @@ def add_test_categories(cat):
     cat = test_categories.validate(cat, True)
 
     def impl(func):
-        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
+        if isinstance(func, type) and issubclass(func, unittest.TestCase):
             raise Exception(
                 "@add_test_categories can only be used to decorate a test method"
             )
@@ -465,7 +465,7 @@ def should_skip_benchmarks_test():
 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):
+    if isinstance(func, type) and issubclass(func, unittest.TestCase):
         raise Exception(
             "@no_debug_info_test can only be used to decorate a test method"
         )
@@ -631,7 +631,7 @@ def is_out_of_tree_debugserver():
 
 def skipIfRemote(func):
     """Decorate the item to skip tests if testing remotely."""
-    return unittest2.skipIf(lldb.remote_platform, "skip on remote platform")(func)
+    return unittest.skipIf(lldb.remote_platform, "skip on remote platform")(func)
 
 
 def skipIfNoSBHeaders(func):
@@ -768,7 +768,7 @@ def skipUnlessDarwin(func):
 
 
 def skipUnlessTargetAndroid(func):
-    return unittest2.skipUnless(
+    return unittest.skipUnless(
         lldbplatformutil.target_is_android(), "requires target to be Android"
     )(func)
 
@@ -809,7 +809,7 @@ 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(
+    return unittest.skipIf(
         lldbplatformutil.getPlatform() in oslist, "skip on %s" % (", ".join(oslist))
     )
 
@@ -818,7 +818,7 @@ 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(
+    return unittest.skipUnless(
         lldbplatformutil.getPlatform() in oslist,
         "requires one of %s" % (", ".join(oslist)),
     )
@@ -1078,7 +1078,7 @@ def _get_bool_config(key, fail_value=True):
 
 def _get_bool_config_skip_if_decorator(key):
     have = _get_bool_config(key)
-    return unittest2.skipIf(not have, "requires " + key)
+    return unittest.skipIf(not have, "requires " + key)
 
 
 def skipIfCursesSupportMissing(func):
@@ -1110,7 +1110,7 @@ def skipIfLLVMTargetMissing(target):
             found = True
             break
 
-    return unittest2.skipIf(not found, "requires " + target)
+    return unittest.skipIf(not found, "requires " + target)
 
 
 # Call sysctl on darwin to see if a specified hardware feature is available on this machine.
diff --git a/lldb/packages/Python/lldbsuite/test/dotest.py b/lldb/packages/Python/lldbsuite/test/dotest.py
index 4393e0caacaab..e6d9a2efb3159 100644
--- a/lldb/packages/Python/lldbsuite/test/dotest.py
+++ b/lldb/packages/Python/lldbsuite/test/dotest.py
@@ -33,7 +33,7 @@
 import tempfile
 
 # Third-party modules
-import unittest2
+import unittest
 
 # LLDB Modules
 import lldbsuite
@@ -658,7 +658,7 @@ def iter_filters():
     for filterspec in iter_filters():
         filtered = True
         print("adding filter spec %s to module %s" % (filterspec, repr(module)))
-        tests = unittest2.defaultTestLoader.loadTestsFromName(filterspec, module)
+        tests = unittest.defaultTestLoader.loadTestsFromName(filterspec, module)
         configuration.suite.addTests(tests)
 
     # Forgo this module if the (base, filterspec) combo is invalid
@@ -670,7 +670,7 @@ def iter_filters():
         # Also the fail-over case when the filterspec branch
         # (base, filterspec) combo doesn't make sense.
         configuration.suite.addTests(
-            unittest2.defaultTestLoader.loadTestsFromName(base)
+            unittest.defaultTestLoader.loadTestsFromName(base)
         )
 
 
@@ -1032,7 +1032,7 @@ def run_suite():
     #
 
     # Install the control-c handler.
-    unittest2.signals.installHandler()
+    unittest.signals.installHandler()
 
     #
     # Invoke the default TextTestRunner to run the test suite
@@ -1066,7 +1066,7 @@ def run_suite():
 
     # Invoke the test runner.
     if configuration.count == 1:
-        result = unittest2.TextTestRunner(
+        result = unittest.TextTestRunner(
             stream=sys.stderr,
             verbosity=configuration.verbose,
             resultclass=test_result.LLDBTestResult,
@@ -1077,7 +1077,7 @@ def run_suite():
         # not enforced.
         test_result.LLDBTestResult.__ignore_singleton__ = True
         for i in range(configuration.count):
-            result = unittest2.TextTestRunner(
+            result = unittest.TextTestRunner(
                 stream=sys.stderr,
                 verbosity=configuration.verbose,
                 resultclass=test_result.LLDBTestResult,
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index d944b09cbcc47..018f2a06980a8 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -44,7 +44,7 @@
 import traceback
 
 # Third-party modules
-import unittest2
+import unittest
 
 # LLDB modules
 import lldb
@@ -517,7 +517,7 @@ def builder_module():
     return lldbplatformutil.builder_module()
 
 
-class Base(unittest2.TestCase):
+class Base(unittest.TestCase):
     """
     Abstract base for performing lldb (see TestBase) or other generic tests (see
     BenchBase for one example).  lldbtest.Base works with the test driver to
@@ -1090,17 +1090,14 @@ def markFailure(self):
             # Once by the Python unittest framework, and a second time by us.
             print("FAIL", file=sbuf)
 
-    def markExpectedFailure(self, err, bugnumber):
+    def markExpectedFailure(self, err):
         """Callback invoked when an expected failure/error occurred."""
         self.__expected__ = True
         with recording(self, False) as sbuf:
             # False because there's no need to write "expected failure" to the
             # stderr twice.
             # Once by the Python unittest framework, and a second time by us.
-            if bugnumber is None:
-                print("expected failure", file=sbuf)
-            else:
-                print("expected failure (problem id:" + str(bugnumber) + ")", file=sbuf)
+            print("expected failure", file=sbuf)
 
     def markSkippedTest(self):
         """Callback invoked when a test is skipped."""
@@ -1111,19 +1108,14 @@ def markSkippedTest(self):
             # Once by the Python unittest framework, and a second time by us.
             print("skipped test", file=sbuf)
 
-    def markUnexpectedSuccess(self, bugnumber):
+    def markUnexpectedSuccess(self):
         """Callback invoked when an unexpected success occurred."""
         self.__unexpected__ = True
         with recording(self, False) as sbuf:
             # False because there's no need to write "unexpected success" to the
             # stderr twice.
             # Once by the Python unittest framework, and a second time by us.
-            if bugnumber is None:
-                print("unexpected success", file=sbuf)
-            else:
-                print(
-                    "unexpected success (problem id:" + str(bugnumber) + ")", file=sbuf
-                )
+            print("unexpected success", file=sbuf)
 
     def getRerunArgs(self):
         return " -f %s.%s" % (self.__class__.__name__, self._testMethodName)
@@ -1704,13 +1696,11 @@ def test_method(self, attrvalue=attrvalue):
 
                     xfail_reason = xfail_for_debug_info_cat_fn(cat)
                     if xfail_reason:
-                        test_method = unittest2.expectedFailure(xfail_reason)(
-                            test_method
-                        )
+                        test_method = unittest.expectedFailure(test_method)
 
                     skip_reason = skip_for_debug_info_cat_fn(cat)
                     if skip_reason:
-                        test_method = unittest2.skip(skip_reason)(test_method)
+                        test_method = unittest.skip(skip_reason)(test_method)
 
                     newattrs[method_name] = test_method
 
@@ -2226,7 +2216,7 @@ def completions_match(self, command, completions):
         match_strings = lldb.SBStringList()
         interp.HandleCompletion(command, len(command), 0, -1, match_strings)
         # match_strings is a 1-indexed list, so we have to slice...
-        self.assertItemsEqual(
+        self.assertCountEqual(
             completions, list(match_strings)[1:], "List of returned completion is wrong"
         )
 
diff --git a/lldb/packages/Python/lldbsuite/test/test_result.py b/lldb/packages/Python/lldbsuite/test/test_result.py
index cb84c909c4196..20365f53a6754 100644
--- a/lldb/packages/Python/lldbsuite/test/test_result.py
+++ b/lldb/packages/Python/lldbsuite/test/test_result.py
@@ -12,14 +12,14 @@
 import traceback
 
 # Third-party modules
-import unittest2
+import unittest
 
 # LLDB Modules
 from . import configuration
 from lldbsuite.test_event import build_exception
 
 
-class LLDBTestResult(unittest2.TextTestResult):
+class LLDBTestResult(unittest.TextTestResult):
     """
     Enforce a singleton pattern to allow introspection of test progress.
 
@@ -243,7 +243,7 @@ def addFailure(self, test, err):
         if self.checkExclusion(
             configuration.xfail_tests, test.id()
         ) or self.checkCategoryExclusion(configuration.xfail_categories, test):
-            self.addExpectedFailure(test, err, None)
+            self.addExpectedFailure(test, err)
             return
 
         configuration.sdir_has_content = True
@@ -264,12 +264,12 @@ def addFailure(self, test, err):
                 else:
                     configuration.failures_per_category[category] = 1
 
-    def addExpectedFailure(self, test, err, bugnumber):
+    def addExpectedFailure(self, test, err):
         configuration.sdir_has_content = True
-        super(LLDBTestResult, self).addExpectedFailure(test, err, bugnumber)
+        super(LLDBTestResult, self).addExpectedFailure(test, err)
         method = getattr(test, "markExpectedFailure", None)
         if method:
-            method(err, bugnumber)
+            method(err)
         self.stream.write(
             "XFAIL: LLDB (%s) :: %s\n" % (self._config_string(test), str(test))
         )
@@ -285,12 +285,12 @@ def addSkip(self, test, reason):
             % (self._config_string(test), str(test), reason)
         )
 
-    def addUnexpectedSuccess(self, test, bugnumber):
+    def addUnexpectedSuccess(self, test):
         configuration.sdir_has_content = True
-        super(LLDBTestResult, self).addUnexpectedSuccess(test, bugnumber)
+        super(LLDBTestResult, self).addUnexpectedSuccess(test)
         method = getattr(test, "markUnexpectedSuccess", None)
         if method:
-            method(bugnumber)
+            method()
         self.stream.write(
             "XPASS: LLDB (%s) :: %s\n" % (self._config_string(test), str(test))
         )
diff --git a/lldb/test/API/commands/expression/test/TestExprs.py b/lldb/test/API/commands/expression/test/TestExprs.py
index e95c76b7104c2..0e3d2e6cf41ff 100644
--- a/lldb/test/API/commands/expression/test/TestExprs.py
+++ b/lldb/test/API/commands/expression/test/TestExprs.py
@@ -12,7 +12,7 @@
 """
 
 
-import unittest2
+import unittest
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
@@ -46,7 +46,7 @@ def build_and_run(self):
 
     # llvm.org/pr17135 <rdar://problem/14874559>
     # APFloat::toString does not identify the correct (i.e. least) precision.
-    @unittest2.expectedFailure
+    @unittest.expectedFailure
     def test_floating_point_expr_commands(self):
         self.build_and_run()
 
diff --git a/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py b/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py
index d9b7426b14844..ee597ad2b148c 100644
--- a/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py
+++ b/lldb/test/API/functionalities/breakpoint/thread_plan_user_breakpoint/TestThreadPlanUserBreakpoint.py
@@ -8,7 +8,7 @@
 """
 
 
-import unittest2
+import unittest
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
diff --git a/lldb/test/API/functionalities/jitloader_gdb/TestJITLoaderGDB.py b/lldb/test/API/functionalities/jitloader_gdb/TestJITLoaderGDB.py
index a074215244787..98c0b149003df 100644
--- a/lldb/test/API/functionalities/jitloader_gdb/TestJITLoaderGDB.py
+++ b/lldb/test/API/functionalities/jitloader_gdb/TestJITLoaderGDB.py
@@ -1,7 +1,7 @@
 """Test for the JITLoaderGDB interface"""
 
 
-import unittest2
+import unittest
 import os
 import lldb
 from lldbsuite.test import lldbutil
@@ -14,7 +14,7 @@ class JITLoaderGDBTestCase(TestBase):
         lambda: "Skipped because the test crashes the test runner",
         bugnumber="llvm.org/pr24702",
     )
-    @unittest2.expectedFailure  # llvm.org/pr24702
+    @unittest.expectedFailure  # llvm.org/pr24702
     def test_bogus_values(self):
         """Test that we handle inferior misusing the GDB JIT interface"""
         self.build()
diff --git a/lldb/test/API/functionalities/thread/state/TestThreadStates.py b/lldb/test/API/functionalities/thread/state/TestThreadStates.py
index e128ca84977b4..56954c9f34c7e 100644
--- a/lldb/test/API/functionalities/thread/state/TestThreadStates.py
+++ b/lldb/test/API/functionalities/thread/state/TestThreadStates.py
@@ -3,7 +3,7 @@
 """
 
 
-import unittest2
+import unittest
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
@@ -41,14 +41,14 @@ def test_state_after_continue(self):
     @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24660")
     @expectedFailureNetBSD
     # thread states not properly maintained
-    @unittest2.expectedFailure  # llvm.org/pr16712
+    @unittest.expectedFailure  # llvm.org/pr16712
     def test_state_after_expression(self):
         """Test thread state after expression."""
         self.build()
         self.thread_state_after_expression_test()
 
     # thread states not properly maintained
-    @unittest2.expectedFailure  # llvm.org/pr15824 and <rdar://problem/28557237>
+    @unittest.expectedFailure  # llvm.org/pr15824 and <rdar://problem/28557237>
     @expectedFailureAll(
         oslist=["windows"],
         bugnumber="llvm.org/pr24668: Breakpoints not resolved correctly",
diff --git a/lldb/test/API/functionalities/tty/TestTerminal.py b/lldb/test/API/functionalities/tty/TestTerminal.py
index 457abd7b4a89d..750cdb3fc8361 100644
--- a/lldb/test/API/functionalities/tty/TestTerminal.py
+++ b/lldb/test/API/functionalities/tty/TestTerminal.py
@@ -2,7 +2,7 @@
 Test lldb command aliases.
 """
 
-import unittest2
+import unittest
 import os
 import lldb
 from lldbsuite.test.decorators import *
@@ -17,13 +17,13 @@ class LaunchInTerminalTestCase(TestBase):
     @skipUnlessDarwin
     # If the test is being run under sudo, the spawned terminal won't retain that elevated
     # privilege so it can't open the socket to talk back to the test case
-    @unittest2.skipIf(
+    @unittest.skipIf(
         hasattr(os, "geteuid") and os.geteuid() == 0, "test cannot be run as root"
     )
     # Do we need to disable this test if the testsuite is being run on a remote system?
     # This env var is only defined when the shell is running in a local mac
     # terminal window
-    @unittest2.skipUnless(
+    @unittest.skipUnless(
         "TERM_PROGRAM" in os.environ, "test must be run on local system"
     )
     @no_debug_info_test
diff --git a/lldb/test/API/lang/c/shared_lib/TestSharedLib.py b/lldb/test/API/lang/c/shared_lib/TestSharedLib.py
index 235b9b4ce3442..e0994aae76169 100644
--- a/lldb/test/API/lang/c/shared_lib/TestSharedLib.py
+++ b/lldb/test/API/lang/c/shared_lib/TestSharedLib.py
@@ -1,7 +1,7 @@
 """Test that types defined in shared libraries work correctly."""
 
 
-import unittest2
+import unittest
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
@@ -35,7 +35,7 @@ def t...
[truncated]

Copy link

github-actions bot commented Jan 30, 2024

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

return lit.Test.UNRESOLVED, output
parsed_details[detail_parts[0]] = int(detail_parts[1])

failures = parsed_details.get("failures", 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavidSpickett DavidSpickett changed the title [test] Switch LLDB API tests from vendored unittest2 to unittest [lldb][test] Switch LLDB API tests from vendored unittest2 to unittest Jan 30, 2024
@DavidSpickett
Copy link
Collaborator

I ran this on Arm and AArch64 Linux. One test lldb/test/API/functionalities/breakpoint/hardware_breakpoints/require_hw_breakpoints/TestRequireHWBreakpoints.py was xfailed on AArch64 is now not. Before:

PASS: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_breakpoint (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
XFAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_out (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
XFAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_over (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
XFAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_range (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
XFAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_until (TestRequireHWBreakpoints.BreakpointLocationsTestCase)

After:

PASS: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_breakpoint (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
FAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_out (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
FAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_over (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
FAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_range (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
FAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_until (TestRequireHWBreakpoints.BreakpointLocationsTestCase)

To be honest, the way the XFAIL is written is very strange. We wrap a skipIf around supports_hw_breakpoints then we expectedFailureIfFn on that. Which sounds like we expect failure if we support hardware breakpoints, but that can't be how that works.

Also, I think the XFAIL was added for Arm (32 bit) (30308d1) and did not intend to include AArch64. Whatever the intent was, this now fails on AArch64 so it should be both.

I will see if I can just rewrite the xfail here, because we're definitely doing something weird at the moment.

raise Exception("Decorator can only be used to decorate a test method")

@wraps(func)
def wrapper(*args, **kwargs):
xfail_reason = expected_fn(*args, **kwargs)
if xfail_reason is not None:
xfail_func = unittest2.expectedFailure(func)
xfail_func = unittest.expectedFailure(func)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's something wrong here. With this PR, the tests in lldb/test/API/functionalities/breakpoint/hardware_breakpoints/require_hw_breakpoints/TestRequireHWBreakpoints.py don't xfail, they just fail.

But I presume that expectedFailureIf is working, so perhaps it's this extra wrapper layer doing something different and the behaviour of expectedFailure has changed since.

@DavidSpickett
Copy link
Collaborator

I've simplified that strange xfail in 7565ae6. Still seeing the tests fail instead of xfail though, see my comment.

@rupprecht
Copy link
Collaborator Author

I ran this on Arm and AArch64 Linux. One test lldb/test/API/functionalities/breakpoint/hardware_breakpoints/require_hw_breakpoints/TestRequireHWBreakpoints.py was xfailed on AArch64 is now not. Before:

PASS: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_breakpoint (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
XFAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_out (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
XFAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_over (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
XFAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_range (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
XFAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_until (TestRequireHWBreakpoints.BreakpointLocationsTestCase)

After:

PASS: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_breakpoint (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
FAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_out (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
FAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_over (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
FAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_range (TestRequireHWBreakpoints.BreakpointLocationsTestCase)
FAIL: LLDB (/home/david.spickett/build-llvm-aarch64/bin/clang-aarch64) :: test_step_until (TestRequireHWBreakpoints.BreakpointLocationsTestCase)

To be honest, the way the XFAIL is written is very strange. We wrap a skipIf around supports_hw_breakpoints then we expectedFailureIfFn on that. Which sounds like we expect failure if we support hardware breakpoints, but that can't be how that works.

Also, I think the XFAIL was added for Arm (32 bit) (30308d1) and did not intend to include AArch64. Whatever the intent was, this now fails on AArch64 so it should be both.

I will see if I can just rewrite the xfail here, because we're definitely doing something weird at the moment.

Argh, thanks very much for running those tests -- I had noticed that issue before, but forgot about it when mailing this. The issue is that expectedFailureIfFn inherently doesn't work anymore. I need to remove that usage first. Thankfully it's only used in three test case files.

Versions of both unittest and unittest2 from trunk expect that any xfail annotations are applied at test class instantiation (or something; a Python compiler expert can correct me). But our local copy of unittest2 is forked from an older version which allows the xfail annotation to apply at test runtime. So anything that relies on lazily deciding if a test case should be xfail will not work when unittest tries to eagerly check if something is xfail. expectedFailureIfFn is a decorator that wraps the test case and only calls the fn when the test has just finished setup and is about to start the actual test case.

More details w/ commit & bug links in #73067 (comment).

Anyway, I'll see about landing that first, and hopefully it doesn't interfere w/ this PR much.

@DavidSpickett
Copy link
Collaborator

These tests could change to something like:

def test_...(self):
    if self.supports_hw_breaks():
        self.skip("can't run on a target with hardware breakpoints")

Since you could argue it's not a known failure, it's a test that's totally incompatible with the host, which would usually be a skip.

@JDevlieghere
Copy link
Member

I ran the test suite locally (macOS on arm64) and TestRequireHWBreakpoints.py is the only regression I found, but that's covered by the other PR. I'm happy to go ahead with landing both PRs and fixing the bots if there's fallout.

Copy link
Collaborator

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

@DavidSpickett
Copy link
Collaborator

Please run the python formatter before landing though.

@rupprecht rupprecht force-pushed the lldb-unittest-switch branch from edd3cfa to 193f8ac Compare February 13, 2024 20:53
@rupprecht rupprecht merged commit 5b38615 into llvm:main Feb 13, 2024
@rupprecht
Copy link
Collaborator Author

I got one buildbot email: https://lab.llvm.org/buildbot/#/builders/68/builds/68776

Looks like lldb-dap crashes occasionally, and so the disconnect event doesn't get the response it was expecting. (The buildbot does not enable the dap request/response logging, but I have it enabled internally, so I have the stack trace -- I'll file a bug later). I saw that TestThreadLocal.py was previously ignoring errors during tear down if the test is annotated xfail, but maybe that's a more general difference. So this patch probably isn't the culprit for that crash flakiness, but may cause it to be more visible.

The arm/aarch64 lldb bots have had an ninja: error: empty path error, they seem to be misconfigured to run ninja "" "". For Mac, Green Dragon seems to be down. So I don't have a great signal if this really broke anything. I'll look at making TestRequireHWBreakpoints.py use skip instead of xfail, I think that makes sense anyway.

@DavidSpickett
Copy link
Collaborator

Yeah now was a bad time to land this unfortunately, because I managed to screw up Linaro's lldb bots. Waiting on a fix for that to go in and we'll get back to you if stuff is broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants