Skip to content
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

[lldb-dap] Fix test_exit_status_message_sigterm test. #90223

Merged
merged 1 commit into from
May 3, 2024

Conversation

mbucko
Copy link
Contributor

@mbucko mbucko commented Apr 26, 2024

Summary:
'test_exit_status_message_sigterm' is failing due to 'psutil' dependency introduced in PR #89405. This fix removes 'deque' dependency and checks if 'psutil' can be imported before running the test. If 'psutil' cannot be imported, it emits a warning and skips the test.

Test Plan:
./bin/llvm-lit -sv /path-to-llvm-project/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py --filter=tools/lldb-dap/console/TestDAP_console.py

Reviewers:
@jeffreytan81,@clayborg,@kusmour, @JDevlieghere,@walter-erquinigo

Subscribers:

Tasks:
lldb-dap

Tags:

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 26, 2024

@llvm/pr-subscribers-lldb

Author: Miro Bucko (mbucko)

Changes

Summary:
'test_exit_status_message_sigterm' is failing due to 'psutil' dependency introduced in PR #89405. This fix removes 'deque' dependency and checks if 'psutil' can be imported before running the test. If 'psutil' cannot be imported, it emits a warning and skips the test.

Test Plan:
./bin/llvm-lit -sv /path-to-llvm-project/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py --filter=tools/lldb-dap/console/TestDAP_console.py

Reviewers:
@jeffreytan81,@clayborg,@kusmour, @JDevlieghere,@walter-erquinigo

Subscribers:

Tasks:
lldb-dap

Tags:


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

1 Files Affected:

  • (modified) lldb/test/API/tools/lldb-dap/console/TestDAP_console.py (+14-6)
diff --git a/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py b/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py
index 8f456aaf890c7f..6fc044c02244b0 100644
--- a/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py
+++ b/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py
@@ -4,17 +4,15 @@
 
 import dap_server
 import lldbdap_testcase
-import psutil
-from collections import deque
 from lldbsuite.test import lldbutil
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 
 
-def get_subprocess(process_name):
-    queue = deque([psutil.Process(os.getpid())])
+def get_subprocess(root_process, process_name):
+    queue = [root_process]
     while queue:
-        process = queue.popleft()
+        process = queue.pop()
         if process.name() == process_name:
             return process
         queue.extend(process.children())
@@ -131,7 +129,17 @@ def test_exit_status_message_sigterm(self):
         process_name = (
             "debugserver" if platform.system() in ["Darwin"] else "lldb-server"
         )
-        process = get_subprocess(process_name)
+
+        try:
+            import psutil
+        except ImportError:
+            print(
+                "psutil not installed, please install using 'pip install psutil'. "
+                "Skipping test_exit_status_message_sigterm test.",
+                file=sys.stderr
+            )
+            return
+        process = get_subprocess(psutil.Process(os.getpid()), process_name)
         process.terminate()
         process.wait()
 

Copy link

github-actions bot commented Apr 26, 2024

✅ With the latest revision this PR passed the Python code formatter.

@walter-erquinigo
Copy link
Member

What about trying to use a python builtin like https://docs.python.org/3/library/multiprocessing.html#multiprocessing.active_children?

@mbucko
Copy link
Contributor Author

mbucko commented Apr 26, 2024

What about trying to use a python builtin like https://docs.python.org/3/library/multiprocessing.html#multiprocessing.active_children?

Nice, I will look into it

@mbucko
Copy link
Contributor Author

mbucko commented Apr 26, 2024

What about trying to use a python builtin like https://docs.python.org/3/library/multiprocessing.html#multiprocessing.active_children?

It seems that multiprocessing doesn't provide a way to iteratively list the subprocesses and there is no other built in python library afaik.
Do you know if it's possible to add 'psutil' as a dependency so that it's installed before the tests are run?

@walter-erquinigo
Copy link
Member

@JDevlieghere , do you know if it's possible to add python dependencies somewhere for lldb tests?

Copy link
Contributor

@jeffreytan81 jeffreytan81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick accept to unblock the build break. Feel free to follow-up in future patches for better solution.

Summary:
'test_exit_status_message_sigterm' is failing due to 'psutil' dependency introduced in PR llvm#89405. This fix removes 'deque' dependency and checks if 'psutil' can be imported before running the test. If 'psutil' cannot be imported, it emits a warning and skips the test.

Test Plan:
./bin/llvm-lit -sv /path-to-llvm-project/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py --filter=tools/lldb-dap/console/TestDAP_console.py

Reviewers:
@jeffreytan81,@clayborg,@kusmour, @JDevlieghere,@walter-erquinigo

Subscribers:

Tasks:
lldb-dap

Tags:
@jeffreytan81 jeffreytan81 merged commit bab1098 into llvm:main May 3, 2024
4 checks passed
@mbucko mbucko deleted the test_fix branch June 12, 2024 04:28
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.

None yet

4 participants