diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1c8471eda..b4380c811 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -317,10 +317,13 @@ jobs: cat /etc/hosts shell: bash + - name: Install Python Dependencies + shell: bash + run: pip install --upgrade --requirement tests/requirements.txt + - name: Test shell: bash run: | - pip install --upgrade --requirement tests/requirements.txt [ "${{ matrix.CC }}" ] && export CC="${{ matrix.CC }}" [ "${{ matrix.CXX }}" ] && export CXX="${{ matrix.CXX }}" pytest --capture=no --verbose tests diff --git a/tests/__init__.py b/tests/__init__.py index 255ce8ff3..a55c5e6f7 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -9,6 +9,7 @@ import pprint import textwrap import socket +import re sourcedir = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) @@ -17,6 +18,129 @@ from tests.assertions import assert_no_proxy_request +def format_error_output(title, command, working_dir, return_code, output=None): + """ + Format detailed error information for failed commands. + + Args: + title: Error title (e.g., "CMAKE CONFIGURE FAILED") + command: Command that failed (list or string) + working_dir: Working directory where command was run + return_code: Return code from the failed command + output: Output from the failed command (optional) + + Returns: + Formatted error message string + """ + if not output: + output = "" + + if not title: + title = "COMMAND FAILED" + + error_details = [] + error_details.append("=" * 60) + error_details.append(title) + error_details.append("=" * 60) + + if isinstance(command, list): + command_str = " ".join(str(arg) for arg in command) + else: + command_str = str(command) + + error_details.append(f"Command: {command_str}") + error_details.append(f"Working directory: {working_dir}") + error_details.append(f"Return code: {return_code}") + + if output: + if isinstance(output, bytes): + output = output.decode("utf-8", errors="replace") + + error_details.append("--- OUTPUT ---") + error_details.append(output.strip()) + + error_details.append("=" * 60) + + # Ensure the error message ends with a newline + return "\n".join(error_details) + "\n" + + +def run_with_capture_on_failure( + command, cwd, env=None, error_title="COMMAND FAILED", failure_exception_class=None +): + """ + Run a subprocess command with output capture, only printing output on failure. + + Args: + command: Command to run (list) + cwd: Working directory + env: Environment variables (optional) + error_title: Title for error reporting (default: "COMMAND FAILED") + failure_exception_class: Exception class to raise on failure (optional) + + Returns: + subprocess.CompletedProcess result on success + + Raises: + failure_exception_class if provided, otherwise subprocess.CalledProcessError + """ + if env is None: + env = os.environ + + process = subprocess.Popen( + command, + cwd=cwd, + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + universal_newlines=True, + bufsize=1, + ) + + # Capture output without streaming + captured_output = [] + try: + for line in process.stdout: + captured_output.append(line) + + return_code = process.wait() + if return_code != 0: + raise subprocess.CalledProcessError(return_code, command) + + # Return a successful result + return subprocess.CompletedProcess( + command, return_code, stdout="".join(captured_output) + ) + + except subprocess.CalledProcessError as e: + # Enhanced error reporting with captured output + error_message = format_error_output( + error_title, command, cwd, e.returncode, "".join(captured_output) + ) + print(error_message, end="", flush=True) + + if failure_exception_class: + raise failure_exception_class("command failed") from None + else: + raise + finally: + # Ensure proper cleanup of the subprocess + if process.poll() is None: + # Process is still running, terminate it + try: + process.terminate() + # Give the process a moment to terminate gracefully + try: + process.wait(timeout=5) + except subprocess.TimeoutExpired: + # Force kill if it doesn't terminate within 5 seconds + process.kill() + process.wait(timeout=1) + except (OSError, ValueError): + # Process might already be terminated or invalid + pass + + def make_dsn(httpserver, auth="uiaeosnrtdy", id=123456, proxy_host=False): url = urllib.parse.urlsplit(httpserver.url_for("/{}".format(id))) # We explicitly use `127.0.0.1` here, because on Windows, `localhost` will @@ -54,8 +178,13 @@ def run(cwd, exe, args, env=dict(os.environ), **kwargs): if os.environ.get("ANDROID_API"): # older android emulators do not correctly pass down the returncode # so we basically echo the return code, and parse it manually - is_pipe = kwargs.get("stdout") == subprocess.PIPE - kwargs["stdout"] = subprocess.PIPE + capture_output = kwargs.get("stdout") != subprocess.PIPE + + if capture_output: + # Capture output for potential display on failure + kwargs["stdout"] = subprocess.PIPE + kwargs["stderr"] = subprocess.STDOUT + child = subprocess.run( [ "{}/platform-tools/adb".format(os.environ["ANDROID_HOME"]), @@ -74,10 +203,33 @@ def run(cwd, exe, args, env=dict(os.environ), **kwargs): **kwargs, ) stdout = child.stdout - child.returncode = int(stdout[stdout.rfind(b"ret:") :][4:]) - child.stdout = stdout[: stdout.rfind(b"ret:")] - if not is_pipe: - sys.stdout.buffer.write(child.stdout) + # Parse return code from Android output using regex + # Handle "ret:NNN" format + match = re.search(rb"ret:(\d+)", stdout) + if match: + child.returncode = int(match.group(1)) + child.stdout = stdout[: match.start()] + else: + # If no ret: pattern found, something is wrong + child.returncode = child.returncode or 1 + child.stdout = stdout + + # Only write output to stdout if not capturing or on success + if not capture_output or child.returncode == 0: + if kwargs.get("stdout") != subprocess.PIPE: + sys.stdout.buffer.write(child.stdout) + elif capture_output and child.returncode != 0: + # Enhanced error reporting for Android test execution failures + command = f"{exe} {' '.join(args)}" + error_message = format_error_output( + "ANDROID TEST EXECUTION FAILED", + command, + "/data/local/tmp", + child.returncode, + child.stdout, + ) + print(error_message, end="", flush=True) + if kwargs.get("check") and child.returncode: raise subprocess.CalledProcessError( child.returncode, child.args, output=child.stdout, stderr=child.stderr @@ -114,14 +266,51 @@ def run(cwd, exe, args, env=dict(os.environ), **kwargs): "--leak-check=yes", *cmd, ] - try: - return subprocess.run([*cmd, *args], cwd=cwd, env=env, **kwargs) - except subprocess.CalledProcessError: - raise pytest.fail.Exception( - "running command failed: {cmd} {args}".format( - cmd=" ".join(cmd), args=" ".join(args) + + # Capture output unless explicitly requested to pipe to caller or stream to stdout + should_capture = kwargs.get("stdout") != subprocess.PIPE and "stdout" not in kwargs + + if should_capture: + # Capture both stdout and stderr for potential display on failure + kwargs_with_capture = kwargs.copy() + kwargs_with_capture["stdout"] = subprocess.PIPE + kwargs_with_capture["stderr"] = subprocess.STDOUT + kwargs_with_capture["universal_newlines"] = True + + try: + result = subprocess.run( + [*cmd, *args], cwd=cwd, env=env, **kwargs_with_capture ) - ) from None + if result.returncode != 0 and kwargs.get("check"): + # Enhanced error reporting for test execution failures + command = cmd + args + error_message = format_error_output( + "TEST EXECUTION FAILED", + command, + cwd, + result.returncode, + result.stdout, + ) + print(error_message, end="", flush=True) + + raise subprocess.CalledProcessError(result.returncode, result.args) + return result + except subprocess.CalledProcessError: + raise pytest.fail.Exception( + "running command failed: {cmd} {args}".format( + cmd=" ".join(cmd), args=" ".join(args) + ) + ) from None + else: + # Use original behavior when stdout is explicitly handled by caller + try: + return subprocess.run([*cmd, *args], cwd=cwd, env=env, **kwargs) + except subprocess.CalledProcessError: + raise pytest.fail.Exception( + "running command failed: {cmd} {args}".format( + cmd=" ".join(cmd), args=" ".join(args) + ) + ) from None def check_output(*args, **kwargs): diff --git a/tests/cmake.py b/tests/cmake.py index 6a7a3ba7d..5e161f36a 100644 --- a/tests/cmake.py +++ b/tests/cmake.py @@ -7,6 +7,7 @@ from pathlib import Path import pytest +from tests import format_error_output, run_with_capture_on_failure class CMake: @@ -91,7 +92,9 @@ def destroy(self): "--merge", coveragedir, *coverage_dirs, - ] + ], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, ) @@ -216,10 +219,12 @@ def cmake(cwd, targets, options=None, cflags=None): config_cmd.append(source_dir) - print("\n{} > {}".format(cwd, " ".join(config_cmd)), flush=True) + # Run with output capture, only print on failure try: - subprocess.run(config_cmd, cwd=cwd, env=env, check=True) - except subprocess.CalledProcessError: + run_with_capture_on_failure( + config_cmd, cwd, env, "CMAKE CONFIGURE FAILED", pytest.fail.Exception + ) + except pytest.fail.Exception: raise pytest.fail.Exception("cmake configure failed") from None # CodeChecker invocations and options are documented here: @@ -241,10 +246,12 @@ def cmake(cwd, targets, options=None, cflags=None): " ".join(buildcmd), ] - print("{} > {}".format(cwd, " ".join(buildcmd)), flush=True) + # Run with output capture, only print on failure try: - subprocess.run(buildcmd, cwd=cwd, check=True) - except subprocess.CalledProcessError: + run_with_capture_on_failure( + buildcmd, cwd, None, "CMAKE BUILD FAILED", pytest.fail.Exception + ) + except pytest.fail.Exception: raise pytest.fail.Exception("cmake build failed") from None # check if the DLL and EXE artifacts contain version-information @@ -302,4 +309,6 @@ def cmake(cwd, targets, options=None, cflags=None): ], cwd=cwd, check=True, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, ) diff --git a/tests/requirements.txt b/tests/requirements.txt index 50002c903..ea6d116e8 100644 --- a/tests/requirements.txt +++ b/tests/requirements.txt @@ -6,3 +6,4 @@ pytest-xdist==3.5.0 clang-format==19.1.3 pywin32==308; sys_platform == "win32" mitmproxy==11.0.0 +psutil==6.0.0 diff --git a/tests/test_enhanced_error_reporting.py b/tests/test_enhanced_error_reporting.py new file mode 100644 index 000000000..a67f7076a --- /dev/null +++ b/tests/test_enhanced_error_reporting.py @@ -0,0 +1,146 @@ +""" +Test enhanced error reporting for cmake failures. + +This test validates that when cmake configure or build fails, detailed error +information is captured and displayed to help with debugging. +""" + +import subprocess +import tempfile +import os +import shutil +import pytest +from .cmake import cmake + + +@pytest.mark.skipif(not shutil.which("cmake"), reason="cmake not available") +def test_cmake_error_reporting(tmp_path): + """Test that cmake failures show detailed error information.""" + # Create a temporary working directory + cwd = tmp_path / "build" + cwd.mkdir() + + # Try to build a non-existent target, which should either: + # - Fail at configure (if dependencies missing) with "cmake configure failed" + # - Fail at build (if dependencies available) with "cmake build failed" + # Both scenarios should show enhanced error reporting + + with pytest.raises(pytest.fail.Exception, match="cmake .* failed"): + cmake(cwd, ["nonexistent_target_that_will_fail"], {}, []) + + +@pytest.mark.skipif(not shutil.which("cmake"), reason="cmake not available") +def test_cmake_successful_configure_shows_no_extra_output(tmp_path): + """Test that successful cmake operations don't show error reporting sections.""" + # This test verifies that our enhanced error reporting doesn't interfere + # with normal operation by running a simple successful case + sourcedir = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) + + cwd = tmp_path / "build" + cwd.mkdir() + + # This should succeed without showing error reporting sections + try: + cmake( + cwd, + ["sentry_test_unit"], + {"SENTRY_BACKEND": "none", "SENTRY_TRANSPORT": "none"}, + [], + ) + # If we get here, the test passed successfully + assert True + except Exception as e: + # If it fails, make sure it's not due to our error reporting + assert "CMAKE CONFIGURE FAILED" not in str(e) + assert "CMAKE BUILD FAILED" not in str(e) + # Re-raise the original exception if it's a different kind of failure + raise + + +def test_enhanced_error_format(): + """Test that the error formatting function creates properly formatted output.""" + from . import format_error_output + + # Test data + mock_cmd = ["cmake", "-DTEST=1", "/some/source"] + mock_cwd = "/some/build/dir" + mock_returncode = 1 + mock_output = ( + "CMake Error: Invalid option TEST\n-- Configuring incomplete, errors occurred!" + ) + + # Use the actual format_error_output function + error_message = format_error_output( + "CMAKE CONFIGURE FAILED", mock_cmd, mock_cwd, mock_returncode, mock_output + ) + + # Verify the formatted output contains expected sections + assert "CMAKE CONFIGURE FAILED" in error_message + assert "Command: cmake -DTEST=1 /some/source" in error_message + assert "Working directory: /some/build/dir" in error_message + assert "Return code: 1" in error_message + assert "--- OUTPUT ---" in error_message + assert "CMake Error: Invalid option TEST" in error_message + assert "-- Configuring incomplete, errors occurred!" in error_message + assert error_message.count("=" * 60) == 3 # Header, title separator, and footer + assert error_message.endswith("\n") # Ensure proper newline ending + + +def test_format_error_output_input_validation(): + """Test input validation of the format_error_output function.""" + from . import format_error_output + + # Test with empty/None output (should handle gracefully) + result = format_error_output("TEST", ["cmd"], "/dir", 1, None) + assert "--- OUTPUT ---" not in result + assert "Return code: 1" in result + + # Test with empty title (should use default) + result = format_error_output("", ["cmd"], "/dir", 1) + assert "COMMAND FAILED" in result + + # Test with string command (should be handled) + result = format_error_output("TEST", "single_command", "/dir", 1) + assert "Command: single_command" in result + + # Test with bytes output (should be decoded) + result = format_error_output("TEST", ["cmd"], "/dir", 1, b"bytes output") + assert "bytes output" in result + + +def test_run_with_capture_on_failure_resource_cleanup(): + """Test that subprocess resources are properly cleaned up even when exceptions occur.""" + from . import run_with_capture_on_failure + import psutil + import os + + # Get initial process count for the current process + initial_children = len(psutil.Process().children(recursive=True)) + + # Test 1: Normal successful command should not leak processes + try: + result = run_with_capture_on_failure( + ["echo", "test"], cwd=".", error_title="TEST COMMAND" + ) + assert result.returncode == 0 + assert "test" in result.stdout + except Exception: + pass # Platform differences shouldn't fail this test + + # Test 2: Command that fails should not leak processes + try: + run_with_capture_on_failure( + ["nonexistent_command_that_will_fail"], cwd=".", error_title="TEST COMMAND" + ) + except (subprocess.CalledProcessError, FileNotFoundError, OSError): + pass # Expected to fail + + # Check that no child processes are left hanging + final_children = len(psutil.Process().children(recursive=True)) + + # The number of child processes should be the same or very close + # Allow for slight variations due to system background processes + assert abs(final_children - initial_children) <= 1, ( + f"Process leak detected: started with {initial_children} children, " + f"ended with {final_children} children" + )