Fix arq quoting to work in runInTerminal (#1981)

* Fix arq quoting to work in runInTerminal

* Default was backwards

* Fix ruff errors

* Fix failing tests

* Only strip quotes on the exe

* Try fixing gw worker failures

* Skip certain test because of cmd limitations

* Need to skip all 'code' based tests on windows

Author: rchiodo

CONTRIBUTING.md

@@ -93,6 +93,8 @@ The tests are run concurrently, and the default number of workers is 8. You can
 
 While tox is the recommended way to run the test suite, pytest can also be invoked directly from the root of the repository. This requires packages in tests/requirements.txt to be installed first.
 
+Using a venv created by tox in the '.tox' folder can make it easier to get the pytest configuration correct. Debugpy needs to be installed into the venv for the tests to run, so using the tox generated .venv makes that easier.
+
 #### Keeping logs on test success
 
 There's an internal setting `debugpy_log_passed` that if set to true will not erase the logs after a successful test run. Just search for this in the code and remove the code that deletes the logs on success.

pytest.ini

@@ -1,5 +1,5 @@
 [pytest]
 testpaths=tests
-timeout=60
+timeout=120
 timeout_method=thread
 addopts=-n8

src/debugpy/adapter/launchers.py

@@ -153,6 +153,23 @@ def on_launcher_connected(sock):
                 request_args["cwd"] = cwd
             if shell_expand_args:
                 request_args["argsCanBeInterpretedByShell"] = True
+
+                # VS Code debugger extension may pass us an argument indicating the 
+                # quoting character to use in the terminal. Otherwise default based on platform.
+                default_quote = '"' if os.name != "nt" else "'"
+                quote_char = arguments["terminalQuoteCharacter"] if "terminalQuoteCharacter" in arguments else default_quote
+
+                # VS code doesn't quote arguments if `argsCanBeInterpretedByShell` is true,
+                # so we need to do it ourselves for the arguments up to the call to the adapter.
+                args = request_args["args"]
+                for i in range(len(args)):
+                    if args[i] == "--":
+                        break
+                    s = args[i]
+                    if " " in s and not ((s.startswith('"') and s.endswith('"')) or (s.startswith("'") and s.endswith("'"))):
+                        s = f"{quote_char}{s}{quote_char}"
+                    args[i] = s
+
             try:
                 # It is unspecified whether this request receives a response immediately, or only
                 # after the spawned command has completed running, so do not block waiting for it.

tests/debug/comms.py

@@ -27,6 +27,7 @@ def listen(self):
         self._server_socket = sockets.create_server("127.0.0.1", 0, self.TIMEOUT)
         _, self.port = sockets.get_address(self._server_socket)
         self._server_socket.listen(0)
+        log.info("{0} created server socket on port {1}", self, self.port)
 
         def accept_worker():
             log.info(
@@ -67,8 +68,14 @@ def _setup_stream(self):
         self._established.set()
 
     def receive(self):
-        self._established.wait()
-        return self._stream.read_json()
+        log.info("{0} waiting for connection to be established...", self)
+        if not self._established.wait(timeout=self.TIMEOUT):
+            log.error("{0} timed out waiting for connection after {1} seconds", self, self.TIMEOUT)
+            raise TimeoutError(f"{self} timed out waiting for debuggee to connect")
+        log.info("{0} connection established, reading JSON...", self)
+        result = self._stream.read_json()
+        log.info("{0} received: {1}", self, result)
+        return result
 
     def send(self, value):
         self.session.timeline.unfreeze()

tests/debug/session.py

@@ -281,7 +281,11 @@ def __exit__(self, exc_type, exc_val, exc_tb):
 
         if self.adapter_endpoints is not None and self.expected_exit_code is not None:
             log.info("Waiting for {0} to close listener ports ...", self.adapter_id)
+            timeout_start = time.time()
             while self.adapter_endpoints.check():
+                if time.time() - timeout_start > 10:
+                    log.warning("{0} listener ports did not close within 10 seconds", self.adapter_id)
+                    break
                 time.sleep(0.1)
 
         if self.adapter is not None:
@@ -290,8 +294,20 @@ def __exit__(self, exc_type, exc_val, exc_tb):
                 self.adapter_id,
                 self.adapter.pid,
             )
-            self.adapter.wait()
-            watchdog.unregister_spawn(self.adapter.pid, self.adapter_id)
+            try:
+                self.adapter.wait(timeout=10)
+            except Exception:
+                log.warning("{0} did not exit gracefully within 10 seconds, force-killing", self.adapter_id)
+                try:
+                    self.adapter.kill()
+                    self.adapter.wait(timeout=5)
+                except Exception as e:
+                    log.error("Failed to force-kill {0}: {1}", self.adapter_id, e)
+            
+            try:
+                watchdog.unregister_spawn(self.adapter.pid, self.adapter_id)
+            except Exception as e:
+                log.warning("Failed to unregister adapter spawn: {0}", e)
             self.adapter = None
 
         if self.backchannel is not None:
@@ -366,9 +382,23 @@ def _make_env(self, base_env, codecov=True):
         return env
 
     def _make_python_cmdline(self, exe, *args):
-        return [
-            str(s.strpath if isinstance(s, py.path.local) else s) for s in [exe, *args]
-        ]
+        def normalize(s, strip_quotes=False):
+            # Convert py.path.local to string
+            if isinstance(s, py.path.local):
+                s = s.strpath
+            else:
+                s = str(s)
+            # Strip surrounding quotes if requested
+            if strip_quotes and len(s) >= 2 and " " in s and (s[0] == s[-1] == '"' or s[0] == s[-1] == "'"):
+                s = s[1:-1]
+            return s
+
+        # Strip quotes from exe
+        result = [normalize(exe, strip_quotes=True)]
+        for arg in args:
+            # Don't strip quotes on anything except the exe
+            result.append(normalize(arg, strip_quotes=False))
+        return result
 
     def spawn_debuggee(self, args, cwd=None, exe=sys.executable, setup=None):
         assert self.debuggee is None

tests/debugpy/test_args.py

@@ -2,6 +2,8 @@
 # Licensed under the MIT License. See LICENSE in the project root
 # for license information.
 
+import os
+import sys
 import pytest
 
 from debugpy.common import log
@@ -35,9 +37,15 @@ def code_to_debug():
 @pytest.mark.parametrize("target", targets.all)
 @pytest.mark.parametrize("run", runners.all_launch)
 @pytest.mark.parametrize("expansion", ["preserve", "expand"])
-def test_shell_expansion(pyfile, target, run, expansion):
+@pytest.mark.parametrize("python_with_space", [False, True])
+def test_shell_expansion(pyfile, tmpdir, target, run, expansion, python_with_space):
     if expansion == "expand" and run.console == "internalConsole":
         pytest.skip('Shell expansion is not supported for "internalConsole"')
+    
+    # Skip tests with python_with_space=True and target="code" on Windows
+    # because .cmd wrappers cannot properly handle multiline string arguments
+    if (python_with_space and target == targets.Code and sys.platform == "win32"):
+        pytest.skip('Windows .cmd wrapper cannot handle multiline code arguments')
 
     @pyfile
     def code_to_debug():
@@ -57,14 +65,34 @@ def expand(args):
                 args[i] = arg[1:]
         log.info("After expansion: {0}", args)
 
+    captured_run_in_terminal_args = []
+    
     class Session(debug.Session):
         def run_in_terminal(self, args, cwd, env):
+            captured_run_in_terminal_args.append(args[:])  # Capture a copy of the args
             expand(args)
             return super().run_in_terminal(args, cwd, env)
 
     argslist = ["0", "$1", "2"]
     args = argslist if expansion == "preserve" else " ".join(argslist)
+    
     with Session() as session:
+        # Create a Python wrapper with a space in the path if requested
+        if python_with_space:
+            # Create a directory with a space in the name
+            python_dir = tmpdir / "python with space"
+            python_dir.mkdir()
+            
+            if sys.platform == "win32":
+                wrapper = python_dir / "python.cmd"
+                wrapper.write(f'@echo off\n"{sys.executable}" %*')
+            else:
+                wrapper = python_dir / "python.sh"
+                wrapper.write(f'#!/bin/sh\nexec "{sys.executable}" "$@"')
+                os.chmod(wrapper.strpath, 0o777)
+            
+            session.config["python"] = wrapper.strpath
+        
         backchannel = session.open_backchannel()
         with run(session, target(code_to_debug, args=args)):
             pass
@@ -73,3 +101,15 @@ def run_in_terminal(self, args, cwd, env):
 
     expand(argslist)
     assert argv == [some.str] + argslist
+
+    # Verify that the python executable path is correctly quoted if it contains spaces
+    if python_with_space and captured_run_in_terminal_args:
+        terminal_args = captured_run_in_terminal_args[0]
+        log.info("Captured runInTerminal args: {0}", terminal_args)
+        
+        # Check if the python executable (first arg) contains a space
+        python_arg = terminal_args[0]
+        assert "python with space" in python_arg, \
+            f"Expected 'python with space' in python path: {python_arg}"
+        if expansion == "expand":
+            assert (python_arg.startswith('"') or python_arg.startswith("'")), f"Python_arg is not quoted: {python_arg}"

tests/debugpy/test_django.py

@@ -5,7 +5,7 @@
 import pytest
 
 from tests import code, debug, log, net, test_data
-from tests.debug import runners, targets
+from tests.debug import targets
 from tests.patterns import some
 
 pytestmark = pytest.mark.timeout(60)
@@ -25,7 +25,6 @@ class lines:
 
 
 @pytest.fixture
-@pytest.mark.parametrize("run", [runners.launch, runners.attach_connect["cli"]])
 def start_django(run):
     def start(session, multiprocess=False):
         # No clean way to kill Django server, expect non-zero exit code

tests/debugpy/test_flask.py

@@ -6,7 +6,7 @@
 import sys
 
 from tests import code, debug, log, net, test_data
-from tests.debug import runners, targets
+from tests.debug import targets
 from tests.patterns import some
 
 pytestmark = pytest.mark.timeout(60)
@@ -27,7 +27,6 @@ class lines:
 
 
 @pytest.fixture
-@pytest.mark.parametrize("run", [runners.launch, runners.attach_connect["cli"]])
 def start_flask(run):
     def start(session, multiprocess=False):
         # No clean way to kill Flask server, expect non-zero exit code

tests/net.py

@@ -17,7 +17,7 @@
 
 used_ports = set()
 
-def get_test_server_port():
+def get_test_server_port(max_retries=10):
     """Returns a server port number that can be safely used for listening without
     clashing with another test worker process, when running with pytest-xdist.
 
@@ -27,6 +27,9 @@ def get_test_server_port():
 
     Note that if multiple test workers invoke this function with different ranges
     that overlap, conflicts are possible!
+    
+    Args:
+        max_retries: Number of times to retry finding an available port
     """
 
     try:
@@ -39,11 +42,32 @@ def get_test_server_port():
         ), "Unrecognized PYTEST_XDIST_WORKER format"
         n = int(worker_id[2:])
 
+    # Try multiple times to find an available port, with retry logic
+    for attempt in range(max_retries):
+        port = 5678 + (n * 300) + attempt
+        while port in used_ports:
+            port += 1
+        
+        # Verify the port is actually available by trying to bind to it
+        sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+        sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+        try:
+            sock.bind(("127.0.0.1", port))
+            sock.close()
+            used_ports.add(port)
+            log.info("Allocated port {0} for worker {1}", port, n)
+            return port
+        except OSError as e:
+            log.warning("Port {0} unavailable (attempt {1}/{2}): {3}", port, attempt + 1, max_retries, e)
+            sock.close()
+            time.sleep(0.1 * (attempt + 1))  # Exponential backoff
+    
+    # Fall back to original behavior if all retries fail
     port = 5678 + (n * 300)
     while port in used_ports:
         port += 1
     used_ports.add(port)
-
+    log.warning("Using fallback port {0} after {1} retries", port, max_retries)
     return port
 
 

tests/pytest_fixtures.py

@@ -46,19 +46,27 @@ def write_log(filename, data):
 
     session.Session.reset_counter()
 
-    session.Session.tmpdir = long_tmpdir
+    # Add worker-specific isolation for tmpdir and log directory
+    try:
+        worker_id = os.environ.get("PYTEST_XDIST_WORKER", "gw0")
+        worker_suffix = f"_{worker_id}"
+    except Exception:
+        worker_suffix = ""
+
+    session.Session.tmpdir = long_tmpdir / f"session{worker_suffix}"
+    session.Session.tmpdir.ensure(dir=True)
     original_log_dir = log.log_dir
 
     failed = True
     try:
         if log.log_dir is None:
-            log.log_dir = (long_tmpdir / "debugpy_logs").strpath
+            log.log_dir = (long_tmpdir / f"debugpy_logs{worker_suffix}").strpath
         else:
             log_subdir = request.node.nodeid
             log_subdir = log_subdir.replace("::", "/")
             for ch in r":?*|<>":
                 log_subdir = log_subdir.replace(ch, f"&#{ord(ch)};")
-            log.log_dir += "/" + log_subdir
+            log.log_dir += "/" + log_subdir + worker_suffix
 
         try:
             py.path.local(log.log_dir).remove()