Skip to content

[lldb][test][x86_64][win] Split assertion in TestBreakpointConditions #100487

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

Conversation

kendalharland
Copy link
Contributor

@kendalharland kendalharland commented Jul 25, 2024

This PR splits the test assertion that verifies we're on the correct line and have the correct value of val to make the error message more clear. At present it just shows Assertion error: True != False

… clarify failure message

When this test fails we see an assertion error `False != True`
This clarifies the error by showing, for example, if `1 != 3` when
comparing `var` to the string "3".
@llvmbot
Copy link
Member

llvmbot commented Jul 25, 2024

@llvm/pr-subscribers-lldb

Author: Kendal Harland (kendalharland)

Changes

On windows x86_64 this test stops on the set breakpoint when val == 1 when the breakpoint condition is set on the SBBreakpointLocation rather than the SBBreakpoint directly. Setting the condition on the breakpoint itself, seems to fix the issue.

This PR also splits the test assertion that verifies we're on the correct line and have the correct value of val to make the error message more clear. At present it just shows Assertion error: True != False

#100486


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

1 Files Affected:

  • (modified) lldb/test/API/functionalities/breakpoint/breakpoint_conditions/TestBreakpointConditions.py (+9-5)
diff --git a/lldb/test/API/functionalities/breakpoint/breakpoint_conditions/TestBreakpointConditions.py b/lldb/test/API/functionalities/breakpoint/breakpoint_conditions/TestBreakpointConditions.py
index 50ba0317fd094..625ca916d20f1 100644
--- a/lldb/test/API/functionalities/breakpoint/breakpoint_conditions/TestBreakpointConditions.py
+++ b/lldb/test/API/functionalities/breakpoint/breakpoint_conditions/TestBreakpointConditions.py
@@ -155,13 +155,13 @@ def breakpoint_conditions_python(self):
             breakpoint.GetThreadIndex(), 1, "The thread index has been set correctly"
         )
 
+        # Set the condition on the breakpoint.
+        breakpoint.SetCondition("val == 3")
+
         # Get the breakpoint location from breakpoint after we verified that,
         # indeed, it has one location.
         location = breakpoint.GetLocationAtIndex(0)
         self.assertTrue(location and location.IsEnabled(), VALID_BREAKPOINT_LOCATION)
-
-        # Set the condition on the breakpoint location.
-        location.SetCondition("val == 3")
         self.expect(location.GetCondition(), exe=False, startstr="val == 3")
 
         # Now launch the process, and do not stop at entry point.
@@ -176,11 +176,15 @@ def breakpoint_conditions_python(self):
             thread.IsValid(),
             "There should be a thread stopped due to breakpoint condition",
         )
+
         frame0 = thread.GetFrameAtIndex(0)
         var = frame0.FindValue("val", lldb.eValueTypeVariableArgument)
-        self.assertTrue(
-            frame0.GetLineEntry().GetLine() == self.line1 and var.GetValue() == "3"
+        self.assertEqual(
+            frame0.GetLineEntry().GetLine(),
+            self.line1,
+            "The debugger stopped on the correct line",
         )
+        self.assertEqual(var.GetValue(), "3")
 
         # The hit count for the breakpoint should be 1.
         self.assertEqual(breakpoint.GetHitCount(), 1)

@kendalharland kendalharland force-pushed the swiftlang/kendal/fix-test-breakpoint-conditions branch from 11ee934 to 4285ea5 Compare July 25, 2024 00:26
@kendalharland
Copy link
Contributor Author

Uploaded a new patch set after realizing my original commit removed tests for the codepath where SBBReakpointLocation.SetCondition is used, which still needs to be tested on aarch64. The latest commit tests both SBBreakpointLocation.SetCondition and SBBreakpoint.SetCondition

@kendalharland kendalharland force-pushed the swiftlang/kendal/fix-test-breakpoint-conditions branch 2 times, most recently from 25240b8 to beef815 Compare July 25, 2024 00:32
@jimingham
Copy link
Collaborator

jimingham commented Jul 25, 2024

The only way that this could succeed on the breakpoint but fail on the location is that there is more than one location and the one the test is setting it on (the zeroth) is not the one that's hit. Since this test is "does a breakpoint condition on a location work" not "does this breakpoint always only produce one location" it might be better to just put the breakpoint on all the locations, and see if the one that is hit does work.

@kendalharland kendalharland force-pushed the swiftlang/kendal/fix-test-breakpoint-conditions branch from beef815 to 29d5a57 Compare July 26, 2024 17:55
@kendalharland
Copy link
Contributor Author

kendalharland commented Jul 26, 2024

The only way that this could succeed on the breakpoint but fail on the location is that there is more than one location and the one the test is setting it on (the zeroth) is not the one that's hit. Since this test is "does a breakpoint condition on a location work" not "does this breakpoint always only produce one location" it might be better to just put the breakpoint on all the locations, and see if the one that is hit does work.

Thanks for the context! In #100477 I learned that the set of compilers I was using to build LLVM are different from the existing lldb-aarch64-windows builder. After fixing that, I am no longer seeing this test fail. I've removed the top commit and left only the one that splits the test assertion to produce a more clear failure message.

@kendalharland kendalharland changed the title [lldb][test][x86_64][win] Set breakpoint condition on breakpoint instead of location in TestBreakpointConditions [lldb][test][x86_64][win] Split assertion TestBreakpointConditions Jul 26, 2024
@kendalharland kendalharland changed the title [lldb][test][x86_64][win] Split assertion TestBreakpointConditions [lldb][test][x86_64][win] Split assertion in TestBreakpointConditions Jul 26, 2024
@kendalharland
Copy link
Contributor Author

Hi @JDevlieghere, friendly reminder to PTAL when you can.

@DavidSpickett DavidSpickett merged commit 3c3851f into llvm:main Aug 2, 2024
6 checks passed
@kendalharland kendalharland deleted the swiftlang/kendal/fix-test-breakpoint-conditions branch August 8, 2024 16:34
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.

5 participants