Skip to content

[lldb] Add additional assertions to TestVTableValue.test_overwrite_vtable #118719

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 1 commit into from
Dec 5, 2024

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