Move stuff into commands and write tests.

pre_commit/commands.py

@@ -1,19 +1,33 @@
 from __future__ import print_function
 
+import logging
 import os
 import pkg_resources
 import shutil
 import stat
+import subprocess
+import sys
 from asottile.ordereddict import OrderedDict
 from asottile.yaml import ordered_dump
 from asottile.yaml import ordered_load
 from plumbum import local
 
 import pre_commit.constants as C
+from pre_commit import git
+from pre_commit import color
 from pre_commit.clientlib.validate_config import CONFIG_JSON_SCHEMA
 from pre_commit.clientlib.validate_config import load_config
 from pre_commit.jsonschema_extensions import remove_defaults
+from pre_commit.logging_handler import LoggingHandler
 from pre_commit.repository import Repository
+from pre_commit.staged_files_only import staged_files_only
+
+
+logger = logging.getLogger('pre_commit')
+
+COLS = int(subprocess.Popen(['tput', 'cols'], stdout=subprocess.PIPE).communicate()[0])
+
+PASS_FAIL_LENGTH = 6
 
 
 def install(runner):
@@ -127,3 +141,80 @@ def clean(runner):
         shutil.rmtree(runner.hooks_workspace_path)
         print('Cleaned {0}.'.format(runner.hooks_workspace_path))
     return 0
+
+
+def _run_single_hook(runner, repository, hook_id, args, write):
+    if args.all_files:
+        get_filenames = git.get_all_files_matching
+    else:
+        get_filenames = git.get_staged_files_matching
+
+    hook = repository.hooks[hook_id]
+
+    # Print the hook and the dots first in case the hook takes hella long to
+    # run.
+    write(
+        '{0}{1}'.format(
+            hook['name'],
+            '.' * (COLS - len(hook['name']) - PASS_FAIL_LENGTH - 6),
+        ),
+    )
+    sys.stdout.flush()
+
+    retcode, stdout, stderr = repository.run_hook(
+        runner.cmd_runner,
+        hook_id,
+        get_filenames(hook['files'], hook['exclude']),
+    )
+
+    if retcode != repository.hooks[hook_id]['expected_return_value']:
+        retcode = 1
+        print_color = color.RED
+        pass_fail = 'Failed'
+    else:
+        retcode = 0
+        print_color = color.GREEN
+        pass_fail = 'Passed'
+
+    write(color.format_color(pass_fail, print_color, args.color) + '\n')
+
+    if (stdout or stderr) and (retcode or args.verbose):
+        write('\n')
+        for output in (stdout, stderr):
+            if output.strip():
+                write(output.strip() + '\n')
+        write('\n')
+
+    return retcode
+
+
+def _run_hooks(runner, args, write):
+    """Actually run the hooks."""
+    retval = 0
+
+    for repo in runner.repositories:
+        for hook_id in repo.hooks:
+            retval |= _run_single_hook(runner, repo, hook_id, args, write=write)
+
+    return retval
+
+
+def _run_hook(runner, hook_id, args, write):
+    for repo in runner.repositories:
+        if hook_id in repo.hooks:
+            return _run_single_hook(runner, repo, hook_id, args, write=write)
+    else:
+        write('No hook with id `{0}`\n'.format(hook_id))
+        return 1
+
+
+def run(runner, args, write=sys.stdout.write):
+    # Set up our logging handler
+    logger.addHandler(LoggingHandler(args.color))
+    logger.setLevel(logging.INFO)
+
+    with staged_files_only(runner.cmd_runner):
+        if args.hook:
+            return _run_hook(runner, args.hook, args, write=write)
+        else:
+            return _run_hooks(runner, args, write=write)

pre_commit/run.py

@@ -1,103 +1,12 @@
-from __future__ import print_function
-
 import argparse
-import logging
-import subprocess
 import sys
 
 from pre_commit import color
 from pre_commit import commands
-from pre_commit import git
-from pre_commit.logging_handler import LoggingHandler
 from pre_commit.runner import Runner
-from pre_commit.staged_files_only import staged_files_only
 from pre_commit.util import entry
 
 
-logger = logging.getLogger('pre_commit')
-
-COLS = int(subprocess.Popen(['tput', 'cols'], stdout=subprocess.PIPE).communicate()[0])
-
-PASS_FAIL_LENGTH = 6
-
-
-def _run_single_hook(runner, repository, hook_id, args):
-    if args.all_files:
-        get_filenames = git.get_all_files_matching
-    else:
-        get_filenames = git.get_staged_files_matching
-
-    hook = repository.hooks[hook_id]
-
-    # Print the hook and the dots first in case the hook takes hella long to
-    # run.
-    print(
-        '{0}{1}'.format(
-            hook['name'],
-            '.' * (COLS - len(hook['name']) - PASS_FAIL_LENGTH - 6),
-        ),
-        end='',
-    )
-
-    retcode, stdout, stderr = repository.run_hook(
-        runner.cmd_runner,
-        hook_id,
-        get_filenames(hook['files'], hook['exclude']),
-    )
-
-    if retcode != repository.hooks[hook_id]['expected_return_value']:
-        retcode = 1
-        print_color = color.RED
-        pass_fail = 'Failed'
-    else:
-        retcode = 0
-        print_color = color.GREEN
-        pass_fail = 'Passed'
-
-    print(color.format_color(pass_fail, print_color, args.color))
-
-    if (stdout or stderr) and (retcode or args.verbose):
-        print()
-        for output in (stdout, stderr):
-            if output.strip():
-                print(output.strip())
-        print()
-
-    return retcode
-
-
-def run_hooks(runner, args):
-    """Actually run the hooks."""
-    retval = 0
-
-    for repo in runner.repositories:
-        for hook_id in repo.hooks:
-            retval |= _run_single_hook(runner, repo, hook_id, args)
-
-    return retval
-
-
-def run_single_hook(runner, hook_id, args):
-    for repo in runner.repositories:
-        if hook_id in repo.hooks:
-            return _run_single_hook(runner, repo, hook_id, args)
-    else:
-        print('No hook with id `{0}`'.format(hook_id))
-        return 1
-
-
-def _run(runner, args):
-    # Set up our logging handler
-    logger.addHandler(LoggingHandler(args.color))
-    logger.setLevel(logging.INFO)
-
-    with staged_files_only(runner.cmd_runner):
-        if args.hook:
-            return run_single_hook(runner, args.hook, args)
-        else:
-            return run_hooks(runner, args)
-
-
 @entry
 def run(argv):
     parser = argparse.ArgumentParser()
@@ -118,11 +27,11 @@ def run(argv):
         '--all-files', '-a', action='store_true', default=False,
         help='Run on all the files in the repo.',
     )
-    run_parser.add_argument('--verbose', '-v', action='store_true', default=False)
     run_parser.add_argument(
         '--color', default='auto', type=color.use_color,
         help='Whether to use color in output.  Defaults to `auto`',
     )
+    run_parser.add_argument('--verbose', '-v', action='store_true', default=False)
 
     help = subparsers.add_parser('help', help='Show help for a specific command.')
     help.add_argument('help_cmd', nargs='?', help='Command to show help for.')
@@ -143,7 +52,7 @@ def run(argv):
     elif args.command == 'autoupdate':
         return commands.autoupdate(runner)
     elif args.command == 'run':
-        return _run(runner, args)
+        return commands.run(runner, args)
     elif args.command == 'help':
         if args.help_cmd:
             parser.parse_args([args.help_cmd, '--help'])

testing/resources/failing_hook_repo/bin/hook.sh

@@ -0,0 +1,6 @@
+#!/usr/bin/env bash
+
+
+echo 'Fail'
+echo $@
+exit 1

testing/resources/failing_hook_repo/hooks.yaml

@@ -0,0 +1,4 @@
+-   id: failing_hook
+    name: Failing hook
+    entry: bin/hook.sh
+    language: script

testing/resources/prints_cwd_repo/prints_cwd/main.py

@@ -1,5 +0,0 @@
-import os
-
-def func():
-    print os.getcwd()
-    return 0

tests/commands_test.py

@@ -1,3 +1,4 @@
+import mock
 import os
 import os.path
 import pkg_resources
@@ -10,15 +11,10 @@ from plumbum import local
 
 
 import pre_commit.constants as C
+from pre_commit import commands
 from pre_commit import git
 from pre_commit.clientlib.validate_config import CONFIG_JSON_SCHEMA
 from pre_commit.clientlib.validate_config import validate_config_extra
-from pre_commit.commands import autoupdate
-from pre_commit.commands import clean
-from pre_commit.commands import install
-from pre_commit.commands import RepositoryCannotBeUpdatedError
-from pre_commit.commands import uninstall
-from pre_commit.commands import _update_repository
 from pre_commit.jsonschema_extensions import apply_defaults
 from pre_commit.runner import Runner
 from testing.auto_namedtuple import auto_namedtuple
@@ -27,7 +23,7 @@ from testing.util import get_resource_path
 
 def test_install_pre_commit(empty_git_dir):
     runner = Runner(empty_git_dir)
-    ret = install(runner)
+    ret = commands.install(runner)
     assert ret == 0
     assert os.path.exists(runner.pre_commit_path)
     pre_commit_contents = open(runner.pre_commit_path).read()
@@ -40,16 +36,16 @@ def test_install_pre_commit(empty_git_dir):
 
 def test_uninstall_pre_commit_does_not_blow_up_when_not_there(empty_git_dir):
     runner = Runner(empty_git_dir)
-    ret = uninstall(runner)
+    ret = commands.uninstall(runner)
     assert ret == 0
 
 
 def test_uninstall(empty_git_dir):
     runner = Runner(empty_git_dir)
     assert not os.path.exists(runner.pre_commit_path)
-    install(runner)
+    commands.install(runner)
     assert os.path.exists(runner.pre_commit_path)
-    uninstall(runner)
+    commands.uninstall(runner)
     assert not os.path.exists(runner.pre_commit_path)
 
 
@@ -77,14 +73,14 @@ def up_to_date_repo(python_hooks_repo):
 
 def test_up_to_date_repo(up_to_date_repo):
     input_sha = up_to_date_repo.repo_config['sha']
-    ret = _update_repository(up_to_date_repo.repo_config)
+    ret = commands._update_repository(up_to_date_repo.repo_config)
     assert ret['sha'] == input_sha
 
 
 def test_autoupdate_up_to_date_repo(up_to_date_repo):
     before = open(C.CONFIG_FILE).read()
     runner = Runner(up_to_date_repo.python_hooks_repo)
-    ret = autoupdate(runner)
+    ret = commands.autoupdate(runner)
     after = open(C.CONFIG_FILE).read()
     assert ret == 0
     assert before == after
@@ -116,12 +112,12 @@ def out_of_date_repo(python_hooks_repo):
 
 
 def test_out_of_date_repo(out_of_date_repo):
-    ret = _update_repository(out_of_date_repo.repo_config)
+    ret = commands._update_repository(out_of_date_repo.repo_config)
     assert ret['sha'] == out_of_date_repo.head_sha
 
 
 def test_removes_defaults(out_of_date_repo):
-    ret = _update_repository(out_of_date_repo.repo_config)
+    ret = commands._update_repository(out_of_date_repo.repo_config)
     assert 'args' not in ret['hooks'][0]
     assert 'expected_return_value' not in ret['hooks'][0]
 
@@ -129,7 +125,7 @@ def test_removes_defaults(out_of_date_repo):
 def test_autoupdate_out_of_date_repo(out_of_date_repo):
     before = open(C.CONFIG_FILE).read()
     runner = Runner(out_of_date_repo.python_hooks_repo)
-    ret = autoupdate(runner)
+    ret = commands.autoupdate(runner)
     after = open(C.CONFIG_FILE).read()
     assert ret == 0
     assert before != after
@@ -162,14 +158,14 @@ def hook_disappearing_repo(python_hooks_repo):
 
 
 def test_hook_disppearing_repo_raises(hook_disappearing_repo):
-    with pytest.raises(RepositoryCannotBeUpdatedError):
+    with pytest.raises(commands.RepositoryCannotBeUpdatedError):
-        _update_repository(hook_disappearing_repo.repo_config)
+        commands._update_repository(hook_disappearing_repo.repo_config)
 
 
 def test_autoupdate_hook_disappearing_repo(hook_disappearing_repo):
     before = open(C.CONFIG_FILE).read()
     runner = Runner(hook_disappearing_repo.python_hooks_repo)
-    ret = autoupdate(runner)
+    ret = commands.autoupdate(runner)
     after = open(C.CONFIG_FILE).read()
     assert ret == 1
     assert before == after
@@ -177,11 +173,105 @@ def test_autoupdate_hook_disappearing_repo(hook_disappearing_repo):
 
 def test_clean(empty_git_dir):
     os.mkdir(C.HOOKS_WORKSPACE)
-    clean(Runner(empty_git_dir))
+    commands.clean(Runner(empty_git_dir))
     assert not os.path.exists(C.HOOKS_WORKSPACE)
 
 
 def test_clean_empty(empty_git_dir):
     assert not os.path.exists(C.HOOKS_WORKSPACE)
-    clean(Runner(empty_git_dir))
+    commands.clean(Runner(empty_git_dir))
     assert not os.path.exists(C.HOOKS_WORKSPACE)
+
+
+def stage_a_file():
+    local['touch']['foo.py']()
+    local['git']['add', 'foo.py']()
+
+
+def get_write_mock_output(write_mock):
+    return ''.join(call[0][0] for call in write_mock.call_args_list)
+
+
+def test_run_all_hooks_passing(repo_with_passing_hook):
+    stage_a_file()
+    runner = Runner(repo_with_passing_hook)
+    args = auto_namedtuple(
+        all_files=False, color=False, verbose=False, hook=None,
+    )
+    write_mock = mock.Mock()
+    ret = commands.run(runner, args, write=write_mock)
+    assert ret == 0
+    printed = get_write_mock_output(write_mock)
+    assert 'Bash hook' in printed
+    assert 'Passed' in printed
+
+
+def test_verbose_prints_output(repo_with_passing_hook):
+    stage_a_file()
+    runner = Runner(repo_with_passing_hook)
+    args = auto_namedtuple(
+        all_files=False, color=False, verbose=True, hook=None,
+    )
+    write_mock = mock.Mock()
+    ret = commands.run(runner, args, write=write_mock)
+    assert ret == 0
+    printed = get_write_mock_output(write_mock)
+    assert 'foo.py\nHello World\n' in printed
+
+
+def test_run_all_hooks_failing(repo_with_failing_hook):
+    stage_a_file()
+    runner = Runner(repo_with_failing_hook)
+    args = auto_namedtuple(
+        all_files=False, color=False, verbose=False, hook=None,
+    )
+    write_mock = mock.Mock()
+    ret = commands.run(runner, args, write=write_mock)
+    assert ret == 1
+    printed = get_write_mock_output(write_mock)
+    assert 'Failing hook' in printed
+    assert 'Failed' in printed
+    assert 'Fail\nfoo.py\n' in printed
+
+
+def test_run_a_specific_hook(repo_with_passing_hook):
+    stage_a_file()
+    runner = Runner(repo_with_passing_hook)
+    args = auto_namedtuple(
+        all_files=False, color=False, verbose=False, hook='bash_hook',
+    )
+    write_mock = mock.Mock()
+    ret = commands.run(runner, args, write=write_mock)
+    assert ret == 0
+    printed = get_write_mock_output(write_mock)
+    assert 'Bash hook' in printed
+    assert 'Passed' in printed
+
+
+def test_run_a_non_existing_hook(repo_with_passing_hook):
+    stage_a_file()
+    runner = Runner(repo_with_passing_hook)
+    args = auto_namedtuple(
+        all_files=False, color=False, verbose=False, hook='nope',
+    )
+    write_mock = mock.Mock()
+    ret = commands.run(runner, args, write=write_mock)
+    assert ret == 1
+    printed = get_write_mock_output(write_mock)
+    assert 'No hook with id `nope`' in printed
+
+
+def test_run_all_files(repo_with_passing_hook):
+    stage_a_file()
+    runner = Runner(repo_with_passing_hook)
+    args = auto_namedtuple(
+        all_files=True, color=False, verbose=True, hook=None,
+    )
+    write_mock = mock.Mock()
+    ret = commands.run(runner, args, write=write_mock)
+    assert ret == 0
+    printed = get_write_mock_output(write_mock)
+    # These are all the files checked into the repo.
+    # This seems kind of weird but it is because py.test reuses fixtures
+    for filename in 'hooks.yaml bin/hook.sh foo.py dummy'.split():
+        assert filename in printed

tests/conftest.py

@@ -2,8 +2,10 @@ from __future__ import absolute_import
 
 import pytest
 import time
+import yaml
 from plumbum import local
 
+import pre_commit.constants as C
 from pre_commit import git
 from pre_commit.clientlib.validate_config import CONFIG_JSON_SCHEMA
 from pre_commit.clientlib.validate_config import validate_config_extra
@@ -68,6 +70,11 @@ def script_hooks_repo(dummy_git_repo):
     yield _make_repo(dummy_git_repo, 'script_hooks_repo')
 
 
+@pytest.yield_fixture
+def failing_hook_repo(dummy_git_repo):
+    yield _make_repo(dummy_git_repo, 'failing_hook_repo')
+
+
 def _make_config(path, hook_id, file_regex):
     config = {
         'repo': path,
@@ -96,4 +103,26 @@ def config_for_prints_cwd_repo(prints_cwd_repo):
 
 @pytest.yield_fixture
 def config_for_script_hooks_repo(script_hooks_repo):
-    yield _make_config(script_hooks_repo, 'bash_hook', '^$')
+    yield _make_config(script_hooks_repo, 'bash_hook', '')
+
+
+def _make_repo_from_configs(*configs):
+    with open(C.CONFIG_FILE, 'w') as config_file:
+        yaml.dump(
+            configs,
+            stream=config_file,
+            Dumper=yaml.SafeDumper,
+            **C.YAML_DUMP_KWARGS
+        )
+
+
+@pytest.yield_fixture
+def repo_with_passing_hook(config_for_script_hooks_repo, empty_git_dir):
+    _make_repo_from_configs(config_for_script_hooks_repo)
+    yield empty_git_dir
+
+
+@pytest.yield_fixture
+def repo_with_failing_hook(failing_hook_repo, empty_git_dir):
+    _make_repo_from_configs(_make_config(failing_hook_repo, 'failing_hook', ''))
+    yield empty_git_dir