Skip to content

Conversation

@bulbazord
Copy link
Member

If this test fails, you're likely going to see something like "Assertion Error: A != B" which doesn't really give much explanation for why this failed.

Instead of ignoring the error, we should assert that it succeeded. This will lead to a better error message, for example:
AssertionError: 'memory write failed for 0x102d7c018' is not success

…able

If this test fails, you're likely going to see something like "Assertion
Error: A != B" which doesn't really give much explanation for why this
failed.

Instead of ignoring the error, we should assert that it succeeded. This
will lead to a better error message, for example:
`AssertionError: 'memory write failed for 0x102d7c018' is not success`
@bulbazord bulbazord requested review from clayborg and removed request for JDevlieghere December 5, 2024 00:10
@llvmbot llvmbot added the lldb label Dec 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 5, 2024

@llvm/pr-subscribers-lldb

Author: Alex Langford (bulbazord)

Changes

If this test fails, you're likely going to see something like "Assertion Error: A != B" which doesn't really give much explanation for why this failed.

Instead of ignoring the error, we should assert that it succeeded. This will lead to a better error message, for example:
AssertionError: 'memory write failed for 0x102d7c018' is not success


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

1 Files Affected:

  • (modified) lldb/test/API/functionalities/vtable/TestVTableValue.py (+6-2)
diff --git a/lldb/test/API/functionalities/vtable/TestVTableValue.py b/lldb/test/API/functionalities/vtable/TestVTableValue.py
index bfc910614afa9e..f0076ea28f7599 100644
--- a/lldb/test/API/functionalities/vtable/TestVTableValue.py
+++ b/lldb/test/API/functionalities/vtable/TestVTableValue.py
@@ -2,7 +2,6 @@
 Make sure the getting a variable path works and doesn't crash.
 """
 
-
 import lldb
 import lldbsuite.test.lldbutil as lldbutil
 from lldbsuite.test.decorators import *
@@ -142,7 +141,12 @@ def test_overwrite_vtable(self):
             "\x01\x01\x01\x01\x01\x01\x01\x01" if is_64bit else "\x01\x01\x01\x01"
         )
         error = lldb.SBError()
-        process.WriteMemory(vtable_addr, data, error)
+        bytes_written = process.WriteMemory(vtable_addr, data, error)
+
+        self.assertSuccess(error)
+        self.assertGreater(
+            bytes_written, 0, "Failed to overwrite first entry in vtable"
+        )
 
         scribbled_child = vtable.GetChildAtIndex(0)
         self.assertEqual(

@bulbazord bulbazord merged commit abb6919 into llvm:main Dec 5, 2024
9 checks passed
@bulbazord bulbazord deleted the confusing-failure-vtable-test branch December 5, 2024 18:38
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.

3 participants