Merge pull request #896 from pre-commit/cleanup

misc cleanup

pre_commit/file_lock.py

@@ -41,14 +41,14 @@ try:  # pragma: no cover (windows)
             # "Regions should be locked only briefly and should be unlocked
             # before closing a file or exiting the program."
             msvcrt.locking(fileno, msvcrt.LK_UNLCK, _region)
-except ImportError:  # pragma: no cover (posix)
+except ImportError:  # pragma: windows no cover
     import fcntl
 
     @contextlib.contextmanager
     def _locked(fileno, blocked_cb):
         try:
             fcntl.flock(fileno, fcntl.LOCK_EX | fcntl.LOCK_NB)
-        except IOError:
+        except IOError:  # pragma: no cover (tests are single-threaded)
             blocked_cb()
             fcntl.flock(fileno, fcntl.LOCK_EX)
         try:

pre_commit/languages/node.py

@@ -28,7 +28,7 @@ def get_env_patch(venv):
         install_prefix = r'{}\bin'.format(win_venv.strip())
     elif sys.platform == 'win32':  # pragma: no cover
         install_prefix = bin_dir(venv)
-    else:
+    else:  # pragma: windows no cover
         install_prefix = venv
     return (
         ('NODE_VIRTUAL_ENV', venv),

tests/commands/install_uninstall_test.py

@@ -84,7 +84,7 @@ def test_install_refuses_core_hookspath(in_git_dir, store):
     assert install(C.CONFIG_FILE, store)
 
 
-@xfailif_no_symlink  # pragma: no cover (non-windows)
+@xfailif_no_symlink  # pragma: windows no cover
 def test_install_hooks_dead_symlink(in_git_dir, store):
     hook = in_git_dir.join('.git/hooks').ensure_dir().join('pre-commit')
     os.symlink('/fake/baz', hook.strpath)
@@ -421,17 +421,14 @@ def test_replace_old_commit_script(tempdir_factory, store):
         assert NORMAL_PRE_COMMIT_RUN.match(output)
 
 
-def test_uninstall_doesnt_remove_not_our_hooks(tempdir_factory):
-    path = git_dir(tempdir_factory)
-    with cwd(path):
-        mkdirp(os.path.join(path, '.git/hooks'))
-        with io.open(os.path.join(path, '.git/hooks/pre-commit'), 'w') as f:
-            f.write('#!/usr/bin/env bash\necho 1\n')
-        make_executable(f.name)
+def test_uninstall_doesnt_remove_not_our_hooks(in_git_dir):
+    pre_commit = in_git_dir.join('.git/hooks').ensure_dir().join('pre-commit')
+    pre_commit.write('#!/usr/bin/env bash\necho 1\n')
+    make_executable(pre_commit.strpath)
 
-        assert uninstall() == 0
+    assert uninstall() == 0
 
-        assert os.path.exists(os.path.join(path, '.git/hooks/pre-commit'))
+    assert pre_commit.exists()
 
 
 PRE_INSTALLED = re.compile(

tests/commands/run_test.py

@@ -781,8 +781,8 @@ def test_include_exclude_base_case(some_filenames):
     ]
 
 
-@xfailif_no_symlink
-def test_matches_broken_symlink(tmpdir):  # pragma: no cover (non-windows)
+@xfailif_no_symlink  # pragma: windows no cover
+def test_matches_broken_symlink(tmpdir):
     with tmpdir.as_cwd():
         os.symlink('does-not-exist', 'link')
         ret = _filter_by_include_exclude({'link'}, '', '^$')

tests/conftest.py

@@ -65,9 +65,10 @@ def in_tmpdir(tempdir_factory):
 
 @pytest.fixture
 def in_git_dir(tmpdir):
-    with tmpdir.as_cwd():
+    repo = tmpdir.join('repo').ensure_dir()
+    with repo.as_cwd():
         cmd_output('git', 'init')
-        yield tmpdir
+        yield repo
 
 
 def _make_conflict():

tests/git_test.py

@@ -9,45 +9,34 @@ import pytest
 from pre_commit import git
 from pre_commit.error_handler import FatalError
 from pre_commit.util import cmd_output
-from testing.fixtures import git_dir
-from testing.util import cwd
 
 
-def test_get_root_at_root(tempdir_factory):
-    path = git_dir(tempdir_factory)
-    with cwd(path):
-        assert os.path.normcase(git.get_root()) == os.path.normcase(path)
+def test_get_root_at_root(in_git_dir):
+    expected = os.path.normcase(in_git_dir.strpath)
+    assert os.path.normcase(git.get_root()) == expected
 
 
-def test_get_root_deeper(tempdir_factory):
-    path = git_dir(tempdir_factory)
-
-    foo_path = os.path.join(path, 'foo')
-    os.mkdir(foo_path)
-    with cwd(foo_path):
-        assert os.path.normcase(git.get_root()) == os.path.normcase(path)
+def test_get_root_deeper(in_git_dir):
+    expected = os.path.normcase(in_git_dir.strpath)
+    with in_git_dir.join('foo').ensure_dir().as_cwd():
+        assert os.path.normcase(git.get_root()) == expected
 
 
-def test_get_root_not_git_dir(tempdir_factory):
-    with cwd(tempdir_factory.get()):
-        with pytest.raises(FatalError):
-            git.get_root()
+def test_get_root_not_git_dir(in_tmpdir):
+    with pytest.raises(FatalError):
+        git.get_root()
 
 
-def test_get_staged_files_deleted(tempdir_factory):
-    path = git_dir(tempdir_factory)
-    with cwd(path):
-        open('test', 'a').close()
-        cmd_output('git', 'add', 'test')
-        cmd_output('git', 'commit', '-m', 'foo', '--allow-empty')
-        cmd_output('git', 'rm', '--cached', 'test')
-        assert git.get_staged_files() == []
+def test_get_staged_files_deleted(in_git_dir):
+    in_git_dir.join('test').ensure()
+    cmd_output('git', 'add', 'test')
+    cmd_output('git', 'commit', '-m', 'foo', '--allow-empty')
+    cmd_output('git', 'rm', '--cached', 'test')
+    assert git.get_staged_files() == []
 
 
-def test_is_not_in_merge_conflict(tempdir_factory):
-    path = git_dir(tempdir_factory)
-    with cwd(path):
-        assert git.is_in_merge_conflict() is False
+def test_is_not_in_merge_conflict(in_git_dir):
+    assert git.is_in_merge_conflict() is False
 
 
 def test_is_in_merge_conflict(in_merge_conflict):
@@ -114,11 +103,10 @@ def test_parse_merge_msg_for_conflicts(input, expected_output):
     assert ret == expected_output
 
 
-def test_get_changed_files(in_tmpdir):
-    cmd_output('git', 'init', '.')
+def test_get_changed_files(in_git_dir):
     cmd_output('git', 'commit', '--allow-empty', '-m', 'initial commit')
-    open('a.txt', 'a').close()
-    open('b.txt', 'a').close()
+    in_git_dir.join('a.txt').ensure()
+    in_git_dir.join('b.txt').ensure()
     cmd_output('git', 'add', '.')
     cmd_output('git', 'commit', '-m', 'add some files')
     files = git.get_changed_files('HEAD', 'HEAD^')
@@ -143,15 +131,12 @@ def test_zsplit(s, expected):
 
 
 @pytest.fixture
-def non_ascii_repo(tmpdir):
-    repo = tmpdir.join('repo').ensure_dir()
-    with repo.as_cwd():
-        cmd_output('git', 'init', '.')
-        cmd_output('git', 'commit', '--allow-empty', '-m', 'initial commit')
-        repo.join('интервью').ensure()
-        cmd_output('git', 'add', '.')
-        cmd_output('git', 'commit', '--allow-empty', '-m', 'initial commit')
-        yield repo
+def non_ascii_repo(in_git_dir):
+    cmd_output('git', 'commit', '--allow-empty', '-m', 'initial commit')
+    in_git_dir.join('интервью').ensure()
+    cmd_output('git', 'add', '.')
+    cmd_output('git', 'commit', '--allow-empty', '-m', 'initial commit')
+    yield in_git_dir
 
 
 def test_all_files_non_ascii(non_ascii_repo):

tests/languages/python_test.py

@@ -11,7 +11,7 @@ def test_norm_version_expanduser():
     if os.name == 'nt':  # pragma: no cover (nt)
         path = r'~\python343'
         expected_path = r'{}\python343'.format(home)
-    else:  # pragma: no cover (non-nt)
+    else:  # pragma: windows no cover
         path = '~/.pyenv/versions/3.4.3/bin/python'
         expected_path = home + '/.pyenv/versions/3.4.3/bin/python'
     result = python.norm_version(path)

tests/meta_hooks/check_hooks_apply_test.py

@@ -1,10 +1,8 @@
 from pre_commit.meta_hooks import check_hooks_apply
 from testing.fixtures import add_config_to_repo
-from testing.fixtures import git_dir
-from testing.util import cwd
 
 
-def test_hook_excludes_everything(capsys, tempdir_factory, mock_store_dir):
+def test_hook_excludes_everything(capsys, in_git_dir, mock_store_dir):
     config = {
         'repos': [
             {
@@ -19,17 +17,15 @@ def test_hook_excludes_everything(capsys, tempdir_factory, mock_store_dir):
         ],
     }
 
-    repo = git_dir(tempdir_factory)
-    add_config_to_repo(repo, config)
+    add_config_to_repo(in_git_dir.strpath, config)
 
-    with cwd(repo):
-        assert check_hooks_apply.main(()) == 1
+    assert check_hooks_apply.main(()) == 1
 
     out, _ = capsys.readouterr()
     assert 'check-useless-excludes does not apply to this repository' in out
 
 
-def test_hook_includes_nothing(capsys, tempdir_factory, mock_store_dir):
+def test_hook_includes_nothing(capsys, in_git_dir, mock_store_dir):
     config = {
         'repos': [
             {
@@ -44,17 +40,15 @@ def test_hook_includes_nothing(capsys, tempdir_factory, mock_store_dir):
         ],
     }
 
-    repo = git_dir(tempdir_factory)
-    add_config_to_repo(repo, config)
+    add_config_to_repo(in_git_dir.strpath, config)
 
-    with cwd(repo):
-        assert check_hooks_apply.main(()) == 1
+    assert check_hooks_apply.main(()) == 1
 
     out, _ = capsys.readouterr()
     assert 'check-useless-excludes does not apply to this repository' in out
 
 
-def test_hook_types_not_matched(capsys, tempdir_factory, mock_store_dir):
+def test_hook_types_not_matched(capsys, in_git_dir, mock_store_dir):
     config = {
         'repos': [
             {
@@ -69,19 +63,15 @@ def test_hook_types_not_matched(capsys, tempdir_factory, mock_store_dir):
         ],
     }
 
-    repo = git_dir(tempdir_factory)
-    add_config_to_repo(repo, config)
+    add_config_to_repo(in_git_dir.strpath, config)
 
-    with cwd(repo):
-        assert check_hooks_apply.main(()) == 1
+    assert check_hooks_apply.main(()) == 1
 
     out, _ = capsys.readouterr()
     assert 'check-useless-excludes does not apply to this repository' in out
 
 
-def test_hook_types_excludes_everything(
-        capsys, tempdir_factory, mock_store_dir,
-):
+def test_hook_types_excludes_everything(capsys, in_git_dir, mock_store_dir):
     config = {
         'repos': [
             {
@@ -96,17 +86,15 @@ def test_hook_types_excludes_everything(
         ],
     }
 
-    repo = git_dir(tempdir_factory)
-    add_config_to_repo(repo, config)
+    add_config_to_repo(in_git_dir.strpath, config)
 
-    with cwd(repo):
-        assert check_hooks_apply.main(()) == 1
+    assert check_hooks_apply.main(()) == 1
 
     out, _ = capsys.readouterr()
     assert 'check-useless-excludes does not apply to this repository' in out
 
 
-def test_valid_exceptions(capsys, tempdir_factory, mock_store_dir):
+def test_valid_exceptions(capsys, in_git_dir, mock_store_dir):
     config = {
         'repos': [
             {
@@ -142,11 +130,9 @@ def test_valid_exceptions(capsys, tempdir_factory, mock_store_dir):
         ],
     }
 
-    repo = git_dir(tempdir_factory)
-    add_config_to_repo(repo, config)
+    add_config_to_repo(in_git_dir.strpath, config)
 
-    with cwd(repo):
-        assert check_hooks_apply.main(()) == 0
+    assert check_hooks_apply.main(()) == 0
 
     out, _ = capsys.readouterr()
     assert out == ''

tests/meta_hooks/check_useless_excludes_test.py

@@ -1,10 +1,8 @@
 from pre_commit.meta_hooks import check_useless_excludes
 from testing.fixtures import add_config_to_repo
-from testing.fixtures import git_dir
-from testing.util import cwd
 
 
-def test_useless_exclude_global(capsys, tempdir_factory):
+def test_useless_exclude_global(capsys, in_git_dir):
     config = {
         'exclude': 'foo',
         'repos': [
@@ -15,18 +13,16 @@ def test_useless_exclude_global(capsys, tempdir_factory):
         ],
     }
 
-    repo = git_dir(tempdir_factory)
-    add_config_to_repo(repo, config)
+    add_config_to_repo(in_git_dir.strpath, config)
 
-    with cwd(repo):
-        assert check_useless_excludes.main(()) == 1
+    assert check_useless_excludes.main(()) == 1
 
     out, _ = capsys.readouterr()
     out = out.strip()
     assert "The global exclude pattern 'foo' does not match any files" == out
 
 
-def test_useless_exclude_for_hook(capsys, tempdir_factory):
+def test_useless_exclude_for_hook(capsys, in_git_dir):
     config = {
         'repos': [
             {
@@ -36,11 +32,9 @@ def test_useless_exclude_for_hook(capsys, tempdir_factory):
         ],
     }
 
-    repo = git_dir(tempdir_factory)
-    add_config_to_repo(repo, config)
+    add_config_to_repo(in_git_dir.strpath, config)
 
-    with cwd(repo):
-        assert check_useless_excludes.main(()) == 1
+    assert check_useless_excludes.main(()) == 1
 
     out, _ = capsys.readouterr()
     out = out.strip()
@@ -51,7 +45,7 @@ def test_useless_exclude_for_hook(capsys, tempdir_factory):
     assert expected == out
 
 
-def test_useless_exclude_with_types_filter(capsys, tempdir_factory):
+def test_useless_exclude_with_types_filter(capsys, in_git_dir):
     config = {
         'repos': [
             {
@@ -67,11 +61,9 @@ def test_useless_exclude_with_types_filter(capsys, tempdir_factory):
         ],
     }
 
-    repo = git_dir(tempdir_factory)
-    add_config_to_repo(repo, config)
+    add_config_to_repo(in_git_dir.strpath, config)
 
-    with cwd(repo):
-        assert check_useless_excludes.main(()) == 1
+    assert check_useless_excludes.main(()) == 1
 
     out, _ = capsys.readouterr()
     out = out.strip()
@@ -82,7 +74,7 @@ def test_useless_exclude_with_types_filter(capsys, tempdir_factory):
     assert expected == out
 
 
-def test_no_excludes(capsys, tempdir_factory):
+def test_no_excludes(capsys, in_git_dir):
     config = {
         'repos': [
             {
@@ -92,17 +84,15 @@ def test_no_excludes(capsys, tempdir_factory):
         ],
     }
 
-    repo = git_dir(tempdir_factory)
-    add_config_to_repo(repo, config)
+    add_config_to_repo(in_git_dir.strpath, config)
 
-    with cwd(repo):
-        assert check_useless_excludes.main(()) == 0
+    assert check_useless_excludes.main(()) == 0
 
     out, _ = capsys.readouterr()
     assert out == ''
 
 
-def test_valid_exclude(capsys, tempdir_factory):
+def test_valid_exclude(capsys, in_git_dir):
     config = {
         'repos': [
             {
@@ -117,11 +107,9 @@ def test_valid_exclude(capsys, tempdir_factory):
         ],
     }
 
-    repo = git_dir(tempdir_factory)
-    add_config_to_repo(repo, config)
+    add_config_to_repo(in_git_dir.strpath, config)
 
-    with cwd(repo):
-        assert check_useless_excludes.main(()) == 0
+    assert check_useless_excludes.main(()) == 0
 
     out, _ = capsys.readouterr()
     assert out == ''

tests/repository_test.py

@@ -2,7 +2,6 @@ from __future__ import absolute_import
 from __future__ import unicode_literals
 
 import collections
-import io
 import os.path
 import re
 import shutil
@@ -24,7 +23,6 @@ from pre_commit.languages import rust
 from pre_commit.repository import Repository
 from pre_commit.util import cmd_output
 from testing.fixtures import config_with_local_hooks
-from testing.fixtures import git_dir
 from testing.fixtures import make_config_from_repo
 from testing.fixtures import make_repo
 from testing.fixtures import modify_manifest
@@ -96,17 +94,14 @@ def test_python_hook_args_with_spaces(tempdir_factory, store):
     )
 
 
-def test_python_hook_weird_setup_cfg(tempdir_factory, store):
-    path = git_dir(tempdir_factory)
-    with cwd(path):
-        with io.open('setup.cfg', 'w') as setup_cfg:
-            setup_cfg.write('[install]\ninstall_scripts=/usr/sbin\n')
+def test_python_hook_weird_setup_cfg(in_git_dir, tempdir_factory, store):
+    in_git_dir.join('setup.cfg').write('[install]\ninstall_scripts=/usr/sbin')
 
-        _test_hook_repo(
-            tempdir_factory, store, 'python_hooks_repo',
-            'foo', [os.devnull],
-            b"['" + five.to_bytes(os.devnull) + b"']\nHello World\n",
-        )
+    _test_hook_repo(
+        tempdir_factory, store, 'python_hooks_repo',
+        'foo', [os.devnull],
+        b"['" + five.to_bytes(os.devnull) + b"']\nHello World\n",
+    )
 
 
 @xfailif_no_venv
@@ -444,14 +439,12 @@ def _norm_pwd(path):
     )[1].strip()
 
 
-def test_cwd_of_hook(tempdir_factory, store):
+def test_cwd_of_hook(in_git_dir, tempdir_factory, store):
     # Note: this doubles as a test for `system` hooks
-    path = git_dir(tempdir_factory)
-    with cwd(path):
-        _test_hook_repo(
-            tempdir_factory, store, 'prints_cwd_repo',
-            'prints_cwd', ['-L'], _norm_pwd(path) + b'\n',
-        )
+    _test_hook_repo(
+        tempdir_factory, store, 'prints_cwd_repo',
+        'prints_cwd', ['-L'], _norm_pwd(in_git_dir.strpath) + b'\n',
+    )
 
 
 def test_lots_of_files(tempdir_factory, store):
@@ -502,10 +495,8 @@ def test_additional_dependencies_roll_forward(tempdir_factory, store):
         assert 'mccabe' not in cmd_output('pip', 'freeze', '-l')[1]
 
 
-@xfailif_windows_no_ruby
-def test_additional_ruby_dependencies_installed(
-        tempdir_factory, store,
-):  # pragma: no cover (non-windows)
+@xfailif_windows_no_ruby  # pragma: windows no cover
+def test_additional_ruby_dependencies_installed(tempdir_factory, store):
     path = make_repo(tempdir_factory, 'ruby_hooks_repo')
     config = make_config_from_repo(path)
     config['hooks'][0]['additional_dependencies'] = ['thread_safe', 'tins']
@@ -518,10 +509,8 @@ def test_additional_ruby_dependencies_installed(
         assert 'tins' in output
 
 
-@xfailif_broken_deep_listdir
-def test_additional_node_dependencies_installed(
-        tempdir_factory, store,
-):  # pragma: no cover (non-windows)
+@xfailif_broken_deep_listdir  # pragma: windows no cover
+def test_additional_node_dependencies_installed(tempdir_factory, store):
     path = make_repo(tempdir_factory, 'node_hooks_repo')
     config = make_config_from_repo(path)
     # Careful to choose a small package that's not depped by npm

tests/staged_files_only_test.py

@@ -31,14 +31,11 @@ def get_short_git_status():
 
 
 @pytest.fixture
-def foo_staged(tempdir_factory):
-    path = git_dir(tempdir_factory)
-    with cwd(path):
-        with io.open('foo', 'w') as foo_file:
-            foo_file.write(FOO_CONTENTS)
-        cmd_output('git', 'add', 'foo')
-        foo_filename = os.path.join(path, 'foo')
-        yield auto_namedtuple(path=path, foo_filename=foo_filename)
+def foo_staged(in_git_dir):
+    foo = in_git_dir.join('foo')
+    foo.write(FOO_CONTENTS)
+    cmd_output('git', 'add', 'foo')
+    yield auto_namedtuple(path=in_git_dir.strpath, foo_filename=foo.strpath)
 
 
 def _test_foo_state(
@@ -134,13 +131,11 @@ def test_foo_both_modify_conflicting(foo_staged, patch_dir):
 
 
 @pytest.fixture
-def img_staged(tempdir_factory):
-    path = git_dir(tempdir_factory)
-    with cwd(path):
-        img_filename = os.path.join(path, 'img.jpg')
-        shutil.copy(get_resource_path('img1.jpg'), img_filename)
-        cmd_output('git', 'add', 'img.jpg')
-        yield auto_namedtuple(path=path, img_filename=img_filename)
+def img_staged(in_git_dir):
+    img = in_git_dir.join('img.jpg')
+    shutil.copy(get_resource_path('img1.jpg'), img.strpath)
+    cmd_output('git', 'add', 'img.jpg')
+    yield auto_namedtuple(path=in_git_dir.strpath, img_filename=img.strpath)
 
 
 def _test_img_state(path, expected_file='img1.jpg', status='A'):