[lldb][test] Remove expectedFailureIfFn #81703
Conversation
Switching to modern `unittest` in 5b38615 needs xfail annotations to be known prior to test running. In contrast, skipping can happen at any time, even during test execution. Thus, `expectedFailureIfFn` inherently doesn't work. Either we eagerly evaluate the function and use `expectedFailureIf` instead, or we use a skip annotation to lazily evaluate the function and potentially skip the test right before it starts. - For `expectedFailureAndroid`, the intent seems to be that certain tests _should_ work on android, but don't. Thus, xfail is appropriate, to ensure the test is re-enabled once those bugs are ever fixed. - For the other uses in individual tests, those generally seem to be cases where the test environment doesn't support the setup required by the test, and so it isn't meaningful to run the test at all. For those, a drop-in replacement to `skipTestIfFn` works.
|
@llvm/pr-subscribers-lldb Author: Jordan Rupprecht (rupprecht) ChangesSwitching to modern Thus,
Full diff: https://github.com/llvm/llvm-project/pull/81703.diff 4 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/decorators.py b/lldb/packages/Python/lldbsuite/test/decorators.py
index a5e1fa51cf6e63..86594c2b409e79 100644
--- a/lldb/packages/Python/lldbsuite/test/decorators.py
+++ b/lldb/packages/Python/lldbsuite/test/decorators.py
@@ -128,33 +128,6 @@ def expectedFailure_impl(func):
return expectedFailure_impl
-def expectedFailureIfFn(expected_fn, bugnumber=None):
- def expectedFailure_impl(func):
- 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 = unittest.expectedFailure(func)
- xfail_func(*args, **kwargs)
- 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 "which syntax was the original
- # function decorated with?"
- if callable(bugnumber):
- return expectedFailure_impl(bugnumber)
- else:
- return expectedFailure_impl
-
-
def skipTestIfFn(expected_fn, bugnumber=None):
def skipTestIfFn_impl(func):
if isinstance(func, type) and issubclass(func, unittest.TestCase):
@@ -417,8 +390,8 @@ def skipIf(
)
-def _skip_for_android(reason, api_levels, archs):
- def impl(obj):
+def _skip_fn_for_android(reason, api_levels, archs):
+ def impl():
result = lldbplatformutil.match_android_device(
lldbplatformutil.getArchitecture(),
valid_archs=archs,
@@ -549,8 +522,8 @@ def expectedFailureAndroid(bugnumber=None, api_levels=None, archs=None):
arch - A sequence of architecture names specifying the architectures
for which a test is expected to fail. None means all architectures.
"""
- return expectedFailureIfFn(
- _skip_for_android("xfailing on android", api_levels, archs), bugnumber
+ return expectedFailureIf(
+ _skip_fn_for_android("xfailing on android", api_levels, archs)(), bugnumber
)
@@ -612,7 +585,7 @@ def expectedFlakeyNetBSD(bugnumber=None, compilers=None):
def expectedFlakeyAndroid(bugnumber=None, api_levels=None, archs=None):
return expectedFlakey(
- _skip_for_android("flakey on android", api_levels, archs), bugnumber
+ _skip_fn_for_android("flakey on android", api_levels, archs), bugnumber
)
@@ -846,7 +819,7 @@ def skipIfTargetAndroid(bugnumber=None, api_levels=None, archs=None):
for which a test is skipped. None means all architectures.
"""
return skipTestIfFn(
- _skip_for_android("skipping for android", api_levels, archs), bugnumber
+ _skip_fn_for_android("skipping for android", api_levels, archs), bugnumber
)
diff --git a/lldb/test/API/commands/platform/sdk/TestPlatformSDK.py b/lldb/test/API/commands/platform/sdk/TestPlatformSDK.py
index bf79a5bd5537d9..6af5767e26d3e8 100644
--- a/lldb/test/API/commands/platform/sdk/TestPlatformSDK.py
+++ b/lldb/test/API/commands/platform/sdk/TestPlatformSDK.py
@@ -39,8 +39,8 @@ def port_not_available(self):
@no_debug_info_test
@skipUnlessDarwin
- @expectedFailureIfFn(no_debugserver)
- @expectedFailureIfFn(port_not_available)
+ @skipTestIfFn(no_debugserver)
+ @skipTestIfFn(port_not_available)
@skipIfRemote
def test_macos_sdk(self):
self.build()
diff --git a/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/require_hw_breakpoints/TestRequireHWBreakpoints.py b/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/require_hw_breakpoints/TestRequireHWBreakpoints.py
index ae4f7ea071ed08..5325f0f00affb8 100644
--- a/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/require_hw_breakpoints/TestRequireHWBreakpoints.py
+++ b/lldb/test/API/functionalities/breakpoint/hardware_breakpoints/require_hw_breakpoints/TestRequireHWBreakpoints.py
@@ -26,7 +26,7 @@ def test_breakpoint(self):
breakpoint = target.BreakpointCreateByLocation("main.c", 1)
self.assertTrue(breakpoint.IsHardware())
- @expectedFailureIfFn(HardwareBreakpointTestBase.supports_hw_breakpoints)
+ @skipTestIfFn(HardwareBreakpointTestBase.supports_hw_breakpoints)
def test_step_range(self):
"""Test stepping when hardware breakpoints are required."""
self.build()
@@ -49,7 +49,7 @@ def test_step_range(self):
"Could not create hardware breakpoint for thread plan" in error.GetCString()
)
- @expectedFailureIfFn(HardwareBreakpointTestBase.supports_hw_breakpoints)
+ @skipTestIfFn(HardwareBreakpointTestBase.supports_hw_breakpoints)
def test_step_out(self):
"""Test stepping out when hardware breakpoints are required."""
self.build()
@@ -71,7 +71,7 @@ def test_step_out(self):
"Could not create hardware breakpoint for thread plan" in error.GetCString()
)
- @expectedFailureIfFn(HardwareBreakpointTestBase.supports_hw_breakpoints)
+ @skipTestIfFn(HardwareBreakpointTestBase.supports_hw_breakpoints)
def test_step_over(self):
"""Test stepping over when hardware breakpoints are required."""
self.build()
@@ -91,7 +91,7 @@ def test_step_over(self):
# Was reported to sometimes pass on certain hardware.
@skipIf(oslist=["linux"], archs=["arm"])
- @expectedFailureIfFn(HardwareBreakpointTestBase.supports_hw_breakpoints)
+ @skipTestIfFn(HardwareBreakpointTestBase.supports_hw_breakpoints)
def test_step_until(self):
"""Test stepping until when hardware breakpoints are required."""
self.build()
diff --git a/lldb/test/API/functionalities/launch_stop_at_entry/TestStopAtEntry.py b/lldb/test/API/functionalities/launch_stop_at_entry/TestStopAtEntry.py
index 496f9c20c2ce7b..b4e2b39e0d5dbd 100644
--- a/lldb/test/API/functionalities/launch_stop_at_entry/TestStopAtEntry.py
+++ b/lldb/test/API/functionalities/launch_stop_at_entry/TestStopAtEntry.py
@@ -49,15 +49,15 @@ def test_stop_default_platform_async(self):
@skipUnlessDarwin
@skipIfRemote
- @expectedFailureIfFn(no_debugserver)
- @expectedFailureIfFn(port_not_available)
+ @skipTestIfFn(no_debugserver)
+ @skipTestIfFn(port_not_available)
def test_stop_remote_platform_sync(self):
self.do_test_stop_at_entry(True, True)
@skipUnlessDarwin
@skipIfRemote
- @expectedFailureIfFn(no_debugserver)
- @expectedFailureIfFn(port_not_available)
+ @skipTestIfFn(no_debugserver)
+ @skipTestIfFn(port_not_available)
def test_stop_remote_platform_async(self):
self.do_test_stop_at_entry(False, True)
|
|
The arm bots still haven't picked up the change :( Green Dragon is back online, and TestRequireHWBreakpoints.py is failing: https://green.lab.llvm.org/green/view/LLDB/job/as-lldb-cmake/16077/testReport/lldb-api/functionalities_breakpoint_hardware_breakpoints_require_hw_breakpoints/TestRequireHWBreakpoints_py/. This PR should fix that. It would be nice to wait for the arm bots to be back in a normal state, but I also don't want to leave green dragon red for too long, so I should probably land this today even if the other arm bots aren't reloaded. I'm not sure what the buildbot process is; @gkistanova, do you have to manually restart something to pick up config changes? e.g. https://lab.llvm.org/buildbot/#/builders/96/builds/52991 is still running |
|
Yeah given GreenDragon has been red for 20h, I'd like to get this merged sooner rather than later. @DavidSpickett what's the benefit of waiting for the arm bots to come back up? |
Broadly speaking, if a buildbot is down, it interferes with bisection if something unexpected happens, particularly for commits like this and 5b38615 which have a broader impact. So if the buildbot comes back up and TestRequireHWBreakpoints.py is failing, it'd be useful to see if it was because of this commit or something else. ... that's what I figure at least. In hindsight it would have been a good idea to check that buildbots were mostly green before landing the original change. I just assumed things were always green :) I feel relatively confident that this should fix the green dragon builds, and is worth the risk that it may create more of a mess in arm bots (but hopefully not!). Reverting 5b38615 is also an option, but it seems like it has already exposed some crashes in |
If you give me the choice of more potential chaos or less, I'm always going to choose less :) But yes this shouldn't be too difficult to pick out if it does fail. |
|
Hi @rupprecht, It seems like this patch has still not fixed the issue in greendragon and the test is still flaky https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ @JDevlieghere Do you have any idea as to why that is still happening? |
|
I can't imagine how the unit test framework could have made this flaky. It's more likely the test was flakey before and it went unnoticed. Can we (temporarily) disable the DAP test? |
|
I believe the old unittest framework may have been ignoring failures that happen during teardown. I have started looking at #81686 a bit, but +1 to suppressing this temporarily -- I should be able to land that in a bit. |
|
Skipped the test in 65c25a4 Tear down behavior was patched for LLDB back in b1490b6. I'm guessing that explains why we weren't noticing those errors -- they get logged as cleanup_errors, not as errors. (Maybe I'm missing something). By switching to standard unittest and dropping that patch, we're going to catch more tear down issues. |
Switching to modern
unittestin 5b38615 needs xfail annotations to be known prior to test running. In contrast, skipping can happen at any time, even during test execution.Thus,
expectedFailureIfFninherently doesn't work. Either we eagerly evaluate the function and useexpectedFailureIfinstead, or we use a skip annotation to lazily evaluate the function and potentially skip the test right before it starts.expectedFailureAndroid, the intent seems to be that certain tests should work on android, but don't. Thus, xfail is appropriate, to ensure the test is re-enabled once those bugs are ever fixed.skipTestIfFnworks.