[Lldb-commits] [PATCH] D61687: Update Python tests for lldb-server on Windows

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.
Herald added a subscriber: dexonsmith.


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteKill.py:17-23
+        reg_expr = r"^\$X[0-9a-fA-F]+([^#]*)#[0-9A-Fa-f]{2}"
+        triple = self.dbg.GetSelectedPlatform().GetTriple()
+
+        # No signal support on Windwos. Only W* response is sent.
+        if re.match(".*-.*-windows", triple):
+            reg_expr = r"^\$W[0-9a-fA-F]+([^#]*)#[0-9A-Fa-f]{2}"
+
----------------
If I understand correctly, the regexes differ only in the first letter. If that's the case, then you could just factor out the rest of the regex and just compute the letter differently:
```
stop_type = 'W' if windows else 'X'
reg_expr = r"^\${}...".format(stop_type)
```


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteModuleInfo.py:24-27
+        # Replace path separators in the json string either with "\\\\" or "/" on Windows.
+        triple = self.dbg.GetSelectedPlatform().GetTriple()
+        if re.match(".*-.*-windows", triple):
+            module_path = module_path.replace(os.path.sep, '/')
----------------
It sounds like you could just unconditionally replace all backslashes with double-backslashes here. That would result in us also correctly handling posix paths that happen to contain a backslash.


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteProcessInfo.py:178-191
     @skipUnlessPlatform(["linux"])
     @llgs_test
     def test_qProcessInfo_contains_triple_llgs_linux(self):
         self.init_llgs_test()
         self.build()
         self.qProcessInfo_contains_keys(set(['triple']))
 
----------------
I'm not sure why the original test was made linux-specific, but I think you can just drop the os name from the test and have a single test which checks for both the presence of `parent-pid` and `triple`. No `@skipUnless` needed as all platforms that currently have a working lldb-server should support these fields.


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py:208-212
+    # In current implementation of llgs on Windows, as a response to '\x03' packet, the debugger
+    # of the native process will trigger a call to DebugBreakProcess that will create a new thread
+    # to handle the exception debug event. So one more stop thread will be notified to the
+    # delegate, e.g. llgs.  So tests below to assert the stop threads number will all fail.
+    @expectedFailureAll(oslist=["windows"])
----------------
Is this something that we consider to be a bug, or is it just how debugging is supposed to work on windows? If it's a bug then fine, but if not then we might consider adjusting the expectations in the test (or just skipping it).


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemote_qThreadStopInfo.py:146-153
         # All but one thread should report no stop reason.
-        self.assertEqual(no_stop_reason_count, thread_count - 1)
+        triple = self.dbg.GetSelectedPlatform().GetTriple()
+
+        # Consider one more thread created by calling DebugBreakProcess.
+        if re.match(".*-.*-windows", triple):
+            self.assertGreaterEqual(no_stop_reason_count, thread_count - 1)
+        else:
----------------
I think this assertion should just be deleted. If the assertion below (that one thread has a stop reason) is true, then it trivially true that the rest of the threads don't have one. Whether or not a platforms spawns an extra thread does not seem to be relevant for this test.


================
Comment at: packages/Python/lldbsuite/test/tools/lldb-server/inferior-crash/TestGdbRemoteAbort.py:40
 
+    @skipIfWindows # For now the signo in T* packet is always 0.
     @llgs_test
----------------
Do you have any plans for changing this? Given that windows does not support signals, maybe it should stay 0...


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61687/new/

https://reviews.llvm.org/D61687