[Lldb-commits] [PATCH] D72096: [lldb/Command] Add --force option for `watchpoint delete` command

[Frederic Riss via Phabricator via lldb-commits]

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