diff --git a/diff_cover/command_runner.py b/diff_cover/command_runner.py index db49d8e6..32de437d 100644 --- a/diff_cover/command_runner.py +++ b/diff_cover/command_runner.py @@ -1,6 +1,8 @@ import subprocess import sys +GIT_INSTALL_URL = "https://git-scm.com/book/en/v2/Getting-Started-Installing-Git" + class CommandError(Exception): """ @@ -8,6 +10,16 @@ class CommandError(Exception): """ +class ExecutableNotFoundError(CommandError): + """ + Error raised when the executable of a command cannot be found at all. + + This is a `CommandError` so that existing handling keeps working, but a + distinct type so the command line tools can tell "your tooling is not + installed" apart from "the command ran and failed". + """ + + def execute(command, exit_codes=None): """Execute provided command returning the stdout Args: @@ -24,7 +36,12 @@ def execute(command, exit_codes=None): exit_codes = [0] stdout_pipe = subprocess.PIPE - with subprocess.Popen(command, stdout=stdout_pipe, stderr=stdout_pipe) as process: + try: + popen = subprocess.Popen(command, stdout=stdout_pipe, stderr=stdout_pipe) + except FileNotFoundError as exc: + raise ExecutableNotFoundError(_executable_not_found_message(command)) from exc + + with popen as process: try: stdout, stderr = process.communicate() except OSError: @@ -52,6 +69,20 @@ def run_command_for_code(command): return process.returncode +def _executable_not_found_message(command): + """ + Explain that the executable of `command` is not installed or not on PATH. + """ + executable = _ensure_unicode(command[0]) if command else "" + message = ( + f"'{executable}' was not found. diff-cover needs '{executable}' to be " + "installed and on your PATH in order to run." + ) + if executable == "git": + message += f" See {GIT_INSTALL_URL} for installation instructions." + return message + + def _ensure_unicode(text): """ Ensures the text passed in becomes unicode diff --git a/diff_cover/diff_cover_tool.py b/diff_cover/diff_cover_tool.py index 1bf66fc4..ada4e268 100644 --- a/diff_cover/diff_cover_tool.py +++ b/diff_cover/diff_cover_tool.py @@ -7,6 +7,7 @@ import xml.etree.ElementTree as etree from diff_cover import DESCRIPTION, VERSION +from diff_cover.command_runner import ExecutableNotFoundError from diff_cover.config_parser import Tool, get_config from diff_cover.diff_reporter import GitDiffReporter from diff_cover.git_diff import GitDiffFileTool, GitDiffTool @@ -418,7 +419,11 @@ def main(argv=None, directory=None): level = logging.ERROR if quiet else logging.WARNING logging.basicConfig(format="%(message)s", level=level) - GitPathTool.set_cwd(directory) + try: + GitPathTool.set_cwd(directory) + except ExecutableNotFoundError as exc: + LOGGER.error("%s", exc) + return 1 fail_under = arg_dict.get("fail_under") diff_tool = None diff --git a/diff_cover/diff_quality_tool.py b/diff_cover/diff_quality_tool.py index 191b4f6d..45a85e14 100644 --- a/diff_cover/diff_quality_tool.py +++ b/diff_cover/diff_quality_tool.py @@ -14,6 +14,7 @@ import diff_cover from diff_cover import hookspecs +from diff_cover.command_runner import ExecutableNotFoundError from diff_cover.config_parser import Tool, get_config from diff_cover.diff_cover_tool import ( COMPARE_BRANCH_HELP, @@ -339,7 +340,11 @@ def main(argv=None, directory=None): level = logging.ERROR if quiet else logging.WARNING logging.basicConfig(format="%(message)s", level=level) - GitPathTool.set_cwd(directory) + try: + GitPathTool.set_cwd(directory) + except ExecutableNotFoundError as exc: + LOGGER.error("%s", exc) + return 1 fail_under = arg_dict.get("fail_under") tool = arg_dict["violations"] user_options = arg_dict.get("options") diff --git a/tests/test_command_runner_missing_executable.py b/tests/test_command_runner_missing_executable.py new file mode 100644 index 00000000..0110fcb6 --- /dev/null +++ b/tests/test_command_runner_missing_executable.py @@ -0,0 +1,92 @@ +# pylint: disable=missing-function-docstring + +"""Tests for the message shown when a required executable is not on PATH. + +See https://github.com/Bachmann1234/diff_cover/issues/303 +""" + +import pytest + +from diff_cover.command_runner import ( + GIT_INSTALL_URL, + CommandError, + ExecutableNotFoundError, + execute, +) + + +@pytest.fixture +def missing_git(mocker): + """Make every subprocess launch fail the way a missing `git` does.""" + return mocker.patch( + "diff_cover.command_runner.subprocess.Popen", + side_effect=FileNotFoundError(2, "No such file or directory", "git"), + ) + + +@pytest.fixture +def failing_git(mocker): + """Make `git` run and fail, which is a different thing entirely.""" + process = mocker.Mock() + process.returncode = 1 + process.communicate.return_value = (b"", b"fatal: not a git repository") + popen = mocker.patch("diff_cover.command_runner.subprocess.Popen") + popen.return_value.__enter__.return_value = process + popen.return_value.__exit__.return_value = None + return popen + + +def test_execute_turns_a_missing_executable_into_a_helpful_error(missing_git): + with pytest.raises(ExecutableNotFoundError) as exc_info: + execute(["git", "rev-parse", "--show-toplevel"]) + + message = str(exc_info.value) + assert "git" in message + assert "PATH" in message + assert message.endswith(f"See {GIT_INSTALL_URL} for installation instructions.") + + +def test_the_new_error_stays_a_command_error(missing_git): + """Callers that already catch CommandError must keep working.""" + assert issubclass(ExecutableNotFoundError, CommandError) + with pytest.raises(CommandError): + execute(["git", "rev-parse", "--show-toplevel"]) + + +def test_execute_does_not_link_git_docs_for_other_executables(mocker): + mocker.patch( + "diff_cover.command_runner.subprocess.Popen", + side_effect=FileNotFoundError(2, "No such file or directory", "pycodestyle"), + ) + + with pytest.raises(ExecutableNotFoundError) as exc_info: + execute(["pycodestyle", "--version"]) + + message = str(exc_info.value) + assert "pycodestyle" in message + assert GIT_INSTALL_URL not in message + + +def test_diff_cover_main_reports_missing_git_without_a_traceback(missing_git, caplog): + from diff_cover.diff_cover_tool import main + + assert main(["diff-cover", "coverage.xml"]) == 1 + assert "PATH" in caplog.text + + +def test_diff_quality_main_reports_missing_git_without_a_traceback(missing_git, caplog): + from diff_cover.diff_quality_tool import main + + assert main(["diff-quality", "--violations", "pycodestyle"]) == 1 + assert "PATH" in caplog.text + + +def test_a_command_that_runs_and_fails_is_left_alone(failing_git): + """Control. Green before and after: only a *missing* binary is reworded.""" + with pytest.raises(CommandError) as exc_info: + execute(["git", "status"]) + + assert not isinstance(exc_info.value, ExecutableNotFoundError) + message = str(exc_info.value) + assert "fatal: not a git repository" in message + assert "PATH" not in message