diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 89197fa0..4906cdcf 100644 --- a/CONTRIBUTING.md +++ b/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. diff --git a/pytest.ini b/pytest.ini index 7f3d7d6b..3439ac33 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,5 +1,5 @@ [pytest] testpaths=tests -timeout=60 +timeout=120 timeout_method=thread addopts=-n8 diff --git a/src/debugpy/adapter/launchers.py b/src/debugpy/adapter/launchers.py index 62f94881..08617851 100644 --- a/src/debugpy/adapter/launchers.py +++ b/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. diff --git a/tests/debug/comms.py b/tests/debug/comms.py index a46ad5ca..f83f90fa 100644 --- a/tests/debug/comms.py +++ b/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() diff --git a/tests/debug/session.py b/tests/debug/session.py index 05abea62..0b2bea26 100644 --- a/tests/debug/session.py +++ b/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 diff --git a/tests/debugpy/test_args.py b/tests/debugpy/test_args.py index 26332552..da83437c 100644 --- a/tests/debugpy/test_args.py +++ b/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}" diff --git a/tests/debugpy/test_django.py b/tests/debugpy/test_django.py index 31571f3e..b0a0862c 100644 --- a/tests/debugpy/test_django.py +++ b/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 diff --git a/tests/debugpy/test_flask.py b/tests/debugpy/test_flask.py index d4a68fc2..1d458268 100644 --- a/tests/debugpy/test_flask.py +++ b/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 diff --git a/tests/net.py b/tests/net.py index f248c1a4..2f468050 100644 --- a/tests/net.py +++ b/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 diff --git a/tests/pytest_fixtures.py b/tests/pytest_fixtures.py index 27d21f1a..6c99b760 100644 --- a/tests/pytest_fixtures.py +++ b/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()