friss added inline comments.
================
Comment at:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py:162-179
+ self.build(dictionary=self.d)
+ self.setTearDownCleanup(dictionary=self.d)
+
+ exe = self.getBuildArtifact(self.exe_name)
+ self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+ # Add a breakpoint to set a watchpoint when stopped on the breakpoint.
----------------
If you cannot reuse another test (see bellow), all the setup can be replaced by
`lldbutil.run_to_line_breakpoint` or `lldbutil.run_to_source_breakpoint`
================
Comment at:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py:194-196
+ # Delete the watchpoint immediately using the force option.
+ self.expect("watchpoint delete --force",
+ substrs=['All watchpoints removed.'])
----------------
Can't we just add this to the end of another test without spinning up a new
process?
Did you verify that the test failed before your patch? I'm asking because I
don't know what m_interpreter.Confirm() does when there's no PTY connected.
Does it default to no or yes?
================
Comment at:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py:199
+ # Use the '-v' option to do verbose listing of the watchpoint.
+ self.runCmd("watchpoint list -v")
+
----------------
You don't test the result of this command, so you don't actually test that
deleting the breakpoints really happened. Is there an SB API to list
watchpoints? If there is, it would be a better fit for this test instead of
matching the output of a command.
================
Comment at:
lldb/packages/Python/lldbsuite/test/commands/watchpoints/watchpoint_commands/TestWatchpointCommands.py:203-206
+ # There should be no more watchpoint hit and the process status should
+ # be 'exited'.
+ self.expect("process status",
+ substrs=['exited'])
----------------
I see, this is the actual test that no watchpoints are present. I'm fine with
adding this new test, I think testing it this way makes sense, but please also
find a way to make sure the list of watchpoints is empty.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72096/new/
https://reviews.llvm.org/D72096
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
