Skip to content

[lldb][Windows] Fixed tests TestPty and TestPtyServer #92090

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 2 commits into from
May 15, 2024

Conversation

slydiman
Copy link
Contributor

The tests TestPty and TestPtyServer use the Unix specific python builtin module termios. They are failed in case of Windows host and Linux target. Disable them for Windows host too.

The tests TestPty and TestPtyServer use the Unix specific python builtin module termios. They are failed in case of Windows host and Linux target. Disable them for Windows host too.
@slydiman slydiman requested a review from JDevlieghere as a code owner May 14, 2024 09:19
@slydiman slydiman requested a review from DavidSpickett May 14, 2024 09:19
@llvmbot llvmbot added the lldb label May 14, 2024
@slydiman slydiman requested a review from labath May 14, 2024 09:19
@llvmbot
Copy link
Member

llvmbot commented May 14, 2024

@llvm/pr-subscribers-lldb

Author: Dmitry Vasilyev (slydiman)

Changes

The tests TestPty and TestPtyServer use the Unix specific python builtin module termios. They are failed in case of Windows host and Linux target. Disable them for Windows host too.


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

2 Files Affected:

  • (modified) lldb/test/API/functionalities/gdb_remote_client/TestPty.py (+1)
  • (modified) lldb/test/API/tools/lldb-server/TestPtyServer.py (+1)
diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestPty.py b/lldb/test/API/functionalities/gdb_remote_client/TestPty.py
index 4d4dd489b294a..9e5c780a24b1b 100644
--- a/lldb/test/API/functionalities/gdb_remote_client/TestPty.py
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestPty.py
@@ -6,6 +6,7 @@
 
 
 @skipIfWindows
+@skipIf(hostoslist=["windows"])
 class TestPty(GDBRemoteTestBase):
     server_socket_class = PtyServerSocket
 
diff --git a/lldb/test/API/tools/lldb-server/TestPtyServer.py b/lldb/test/API/tools/lldb-server/TestPtyServer.py
index aa5bd635650ac..7d91d762cb3ba 100644
--- a/lldb/test/API/tools/lldb-server/TestPtyServer.py
+++ b/lldb/test/API/tools/lldb-server/TestPtyServer.py
@@ -8,6 +8,7 @@
 
 
 @skipIfWindows
+@skipIf(hostoslist=["windows"])
 class PtyServerTestCase(gdbremote_testcase.GdbRemoteTestCaseBase):
     def setUp(self):
         super().setUp()

@labath
Copy link
Collaborator

labath commented May 14, 2024

Maybe the skipIfWindows decorator can also be removed here? We haven't been historically very good at distinguishing windows hosts and targets (nobody cared until now), and from the looks of things, these tests could conceivably work in a linux->windows remote scenario (although they wouldn't really test much given that they use a mock server, so maybe slapping skipIfRemote would be reasonable as well).

@slydiman
Copy link
Contributor Author

Agreed. I have removed @skipIfWindows. @skipIfRemote is too much. These tests are still usable for Linux->Linux and such.

@labath
Copy link
Collaborator

labath commented May 15, 2024

@skipIfRemote is too much. These tests are still usable for Linux->Linux and such.

I don't really care about this, but I'll note that while these tests will run in a remote configuration, they will not actually test any meaningful property of the remote setup. They create a local pty endpoint, connect and talk over it, completely ignoring the the remote connection that the general test infra has set up for them. In fact, I might go so far as to say that if this test does ever fail in a remote configuration, then that's a bug in the test, because it's not insulated enough from the environment.

I realize this if confusing, and that's because the API test suite is really two test suites jumbled into one:

  • on one side we have really generic tests, which test that debugging writ large works, and are usable in a wide variety of configurations. To achieve this, the tests try to make as few assumptions about the environment as possible.
  • on the other one, we have tests for some very specific scenarios/corner cases/bugs/features. Because the thing they are testing occurs only in very specific circumstances, these tests usually try to insulate themselves from the environment as much as possible. And since these things often require very elaborate setups, it's usually not possible or very difficult to test them using one of other other test suites.

We're not currently doing a good job at differentiating the two. It might be the best two split the test suite into two, but that would be a fairly big undertaking, and would involve a lot of hairsplitting, as the line is not always very clear.

@slydiman slydiman merged commit eacefba into llvm:main May 15, 2024
4 checks passed
@slydiman slydiman deleted the fix-lldb-test-TestPty branch July 25, 2024 21:31
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