test_environment_not_sourced fails on my Ubuntu

[yoavcaspi]

During my working on pre-commit I notice that one of the tests fails in tox.

Here is an example

(venv) yocaspi@yocaspi-VirtualBox:~/workspace/pre-commit$ tox -e py27
GLOB sdist-make: /home/yocaspi/workspace/pre-commit/setup.py
py27 inst-nodeps: /home/yocaspi/workspace/pre-commit/.tox/.tmp/package/1/pre_commit-1.16.0.zip
py27 installed: DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.,aspy.yaml==1.2.0,atomicwrites==1.3.0,attrs==19.1.0,cfgv==1.6.0,configparser==3.7.4,contextlib2==0.5.5,coverage==4.5.3,entrypoints==0.3,enum34==1.1.6,flake8==3.7.7,funcsigs==1.0.2,functools32==3.2.3.post2,futures==3.2.0,identify==1.4.2,importlib-metadata==0.9,importlib-resources==1.0.2,mccabe==0.6.1,mock==3.0.4,more-itertools==5.0.0,nodeenv==1.3.3,pathlib2==2.3.3,pluggy==0.9.0,-e git+git@github.com:yoavcaspi/pre-commit.git@217d31ec1cfe2ed67d360016d8b3f13e1ee16c1f#egg=pre_commit,py==1.8.0,pycodestyle==2.5.0,pyflakes==2.1.1,pytest==4.4.1,pytest-env==0.6.2,PyYAML==5.1,scandir==1.10.0,six==1.12.0,toml==0.10.0,typing==3.6.6,virtualenv==16.5.0,zipp==0.4.0
py27 run-test-pre: PYTHONHASHSEED='3947365355'
py27 run-test: commands[0] | coverage erase
py27 run-test: commands[1] | coverage run -m pytest tests
============================= test session starts =============================
platform linux2 -- Python 2.7.15rc1, pytest-4.4.1, py-1.8.0, pluggy-0.9.0
cachedir: .tox/py27/.pytest_cache
rootdir: /home/yocaspi/workspace/pre-commit, inifile: tox.ini
plugins: env-0.6.2
collected 579 items                                                           

tests/clientlib_test.py .....................................           [  6%]
tests/color_test.py ........                                            [  7%]
tests/envcontext_test.py ............                                   [  9%]
tests/error_handler_test.py ......                                      [ 10%]
tests/git_test.py ........................                              [ 15%]
tests/logging_handler_test.py ..                                        [ 15%]
tests/main_test.py .....................                                [ 18%]
tests/make_archives_test.py ..                                          [ 19%]
tests/output_test.py ...........                                        [ 21%]
tests/parse_shebang_test.py .................                           [ 24%]
tests/prefix_test.py .........                                          [ 25%]
tests/repository_test.py ....x..sssss......s........................... [ 33%]
...................                                                     [ 36%]
tests/staged_files_only_test.py .................................       [ 42%]
tests/store_test.py ...................                                 [ 45%]
tests/util_test.py .........                                            [ 47%]
tests/xargs_test.py ......................                              [ 51%]
tests/commands/autoupdate_test.py ..................                    [ 54%]
tests/commands/clean_test.py ..                                         [ 54%]
tests/commands/gc_test.py ........                                      [ 56%]
tests/commands/install_uninstall_test.py ...................F.......... [ 61%]
..................                                                      [ 64%]
tests/commands/migrate_config_test.py ............                      [ 66%]
tests/commands/run_test.py ............................................ [ 74%]
.........................                                               [ 78%]
tests/commands/sample_config_test.py .                                  [ 78%]
tests/commands/try_repo_test.py ......                                  [ 79%]
tests/languages/all_test.py ........................................... [ 87%]
...........................                                             [ 91%]
tests/languages/docker_test.py .                                        [ 91%]
tests/languages/golang_test.py ........                                 [ 93%]
tests/languages/helpers_test.py ..........                              [ 94%]
tests/languages/pygrep_test.py ..........                               [ 96%]
tests/languages/python_test.py ......                                   [ 97%]
tests/languages/ruby_test.py ..                                         [ 98%]
tests/meta_hooks/check_hooks_apply_test.py .....                        [ 98%]
tests/meta_hooks/check_useless_excludes_test.py .....                   [ 99%]
tests/meta_hooks/identity_test.py .                                     [100%]

================================== FAILURES ===================================
________________________ test_environment_not_sourced _________________________

tempdir_factory = <tests.conftest.TmpdirFactory object at 0x7fb8f0886110>
store = <pre_commit.store.Store object at 0x7fb8f0886910>

    def test_environment_not_sourced(tempdir_factory, store):
        path = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
        with cwd(path):
            # Patch the executable to simulate rming virtualenv
            with mock.patch.object(sys, 'executable', '/does-not-exist'):
                assert install(C.CONFIG_FILE, store) == 0
    
            # Use a specific homedir to ignore --user installs
            homedir = tempdir_factory.get()
            ret, stdout, stderr = git_commit(
                env={
                    'HOME': homedir,
                    'PATH': _path_without_us(),
                    # Git needs this to make a commit
                    'GIT_AUTHOR_NAME': os.environ['GIT_AUTHOR_NAME'],
                    'GIT_COMMITTER_NAME': os.environ['GIT_COMMITTER_NAME'],
                    'GIT_AUTHOR_EMAIL': os.environ['GIT_AUTHOR_EMAIL'],
                    'GIT_COMMITTER_EMAIL': os.environ['GIT_COMMITTER_EMAIL'],
                },
                retcode=None,
            )
>           assert ret == 1
E           assert 0 == 1

tests/commands/install_uninstall_test.py:264: AssertionError
---------------------------- Captured stdout call -----------------------------
pre-commit installed at .git/hooks/pre-commit
======== 1 failed, 571 passed, 6 skipped, 1 xfailed in 320.69 seconds =========
ERROR: InvocationError for command /home/yocaspi/workspace/pre-commit/.tox/py27/bin/coverage run -m pytest tests (exited with code 1)
___________________________________ summary ___________________________________
ERROR:   py27: commands failed

Here is when I'm using pytest for only this test:

(venv) yocaspi@yocaspi-VirtualBox:~/workspace/pre-commit$ pytest -k test_environment_not_sourced
============================= test session starts =============================
platform linux -- Python 3.7.1, pytest-4.4.1, py-1.8.0, pluggy-0.11.0
rootdir: /home/yocaspi/workspace/pre-commit, inifile: tox.ini
collected 579 items / 578 deselected / 1 selected                             

tests/commands/install_uninstall_test.py F                              [100%]

================================== FAILURES ===================================
________________________ test_environment_not_sourced _________________________

tempdir_factory = <tests.conftest.tempdir_factory.<locals>.TmpdirFactory object at 0x7f9426320208>
store = <pre_commit.store.Store object at 0x7f9426320668>

    def test_environment_not_sourced(tempdir_factory, store):
        path = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
        with cwd(path):
            # Patch the executable to simulate rming virtualenv
            with mock.patch.object(sys, 'executable', '/does-not-exist'):
                assert install(C.CONFIG_FILE, store) == 0
    
            # Use a specific homedir to ignore --user installs
            homedir = tempdir_factory.get()
            ret, stdout, stderr = git_commit(
                env={
                    'HOME': homedir,
                    'PATH': _path_without_us(),
                    # Git needs this to make a commit
>                   'GIT_AUTHOR_NAME': os.environ['GIT_AUTHOR_NAME'],
                    'GIT_COMMITTER_NAME': os.environ['GIT_COMMITTER_NAME'],
                    'GIT_AUTHOR_EMAIL': os.environ['GIT_AUTHOR_EMAIL'],
                    'GIT_COMMITTER_EMAIL': os.environ['GIT_COMMITTER_EMAIL'],
                },
                retcode=None,
            )

tests/commands/install_uninstall_test.py:257: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = environ({'CLUTTER_IM_MODULE': 'xim', 'LS_COLORS': 'rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01...v/bin/pytest', 'PYTEST_CURRENT_TEST': 'tests/commands/install_uninstall_test.py::test_environment_not_sourced (call)'})
key = 'GIT_AUTHOR_NAME'

    def __getitem__(self, key):
        try:
            value = self._data[self.encodekey(key)]
        except KeyError:
            # raise KeyError with the original key value
>           raise KeyError(key) from None
E           KeyError: 'GIT_AUTHOR_NAME'

venv/lib/python3.7/os.py:678: KeyError
---------------------------- Captured stdout call -----------------------------
pre-commit installed at .git/hooks/pre-commit
================== 1 failed, 578 deselected in 0.68 seconds ===================

[asottile]

yeah this test was tricky to write and properly PATH-constrain

the first failure is due to having pre-commit globally installed, the PATH can probably be further constrained to avoid that

The second failure is due to not having pytest-env installed (see requirements-dev.txt)

[yoavcaspi]

regarding the second failure.
I tackled it only since
I was not able to reproduce this step from CONTRIBUTION.md (https://github.com/pre-commit/pre-commit/blob/master/CONTRIBUTING.md#setting-up-an-environment)

Setting up an environment

This is useful for running specific tests. The easiest way to set this up is to run:

    tox -e venv
    . venv-pre_commit/bin/activate

This will create and put you into a virtualenv which has an editable installation of pre-commit. Hack away! Running pre-commit will reflect your changes immediately.

I found out that you delete the [testenv:venv] section in your azure-pipelines commit. any reason to do it?
9c6edab#diff-b91f3d5bd63fcd17221b267e851608e8L15

[asottile]

ah right, I deleted that because I don't use it myself and it defaulted to python 2 which is not great

That can probably be updated to tox -e py37 and . .tox/py37/bin/activate (or some such)

[yoavcaspi]

but the advantage of using this venv was that it didn't run any tests and it was created in the root directory and not inside the .tox directory.
what about this:

[testenv:venv]
basepython = python3.7
envdir = venv-pre-commit
commands =

BTW, If you are not using it, what do you use?

[asottile]

oh, I meant tox -e py37 --notest oops!

[yoavcaspi]

I don't like this proposal.
but I'll change the documentation to fit your proposal because the current documentation is broken.

[asottile]

myself I mostly use

virtualenv venv
. venv/bin/activate
pip install -r requirements-dev.txt

I wonder if @gaborbernat would be ok with me adding some feature to tox that's like tox --envdir=venv-pre_commit --notest (where it'll just use the [testenv] settings and create an environment) (without the configuration needing to exist)

It's probably fine to put back the [testenv:venv] if you think it'll be useful 👍

[yoavcaspi]

sure. Thanks. I'll do it tomorrow.

[gaborbernat]

@asottile I'm fine with that but I think a lot of things are derived from envdir 🤔

[asottile]

I'll see what I come up with 👍

[yoavcaspi]

yeah this test was tricky to write and properly PATH-constrain

the first failure is due to having pre-commit globally installed, the PATH can probably be further constrained to avoid that

The second failure is due to not having pytest-env installed (see requirements-dev.txt)

regarding the first error.
I don't have pre-commit globally installed as far as I can tell.
I'm using the tox from the tox -e venv virtual environment.

Here is some logs that might help:

(venv-pre_commit) yocaspi@yocaspi-VirtualBox:~/workspace/pre-commit$ which pre-commit
/home/yocaspi/workspace/pre-commit/venv-pre_commit/bin/pre-commit
(venv-pre_commit) yocaspi@yocaspi-VirtualBox:~/workspace/pre-commit$ deactivate
yocaspi@yocaspi-VirtualBox:~/workspace/pre-commit$ which pre-commit
yocaspi@yocaspi-VirtualBox:~/workspace/pre-commit$ pre-commit

Command 'pre-commit' not found, but can be installed with:

sudo snap install pre-commit

and the path from the test:

(venv-pre_commit) yocaspi@yocaspi-VirtualBox:~/workspace/pre-commit$ tox -e py27 -- -k test_environment_not_sourced --pdb
GLOB sdist-make: /home/yocaspi/workspace/pre-commit/setup.py
py27 inst-nodeps: /home/yocaspi/workspace/pre-commit/.tox/.tmp/package/1/pre_commit-1.16.1.zip
py27 installed: DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.,aspy.yaml==1.2.0,atomicwrites==1.3.0,attrs==19.1.0,cfgv==1.6.0,configparser==3.7.4,contextlib2==0.5.5,coverage==4.5.3,entrypoints==0.3,enum34==1.1.6,flake8==3.7.7,funcsigs==1.0.2,functools32==3.2.3.post2,futures==3.2.0,identify==1.4.2,importlib-metadata==0.9,importlib-resources==1.0.2,mccabe==0.6.1,mock==3.0.4,more-itertools==5.0.0,nodeenv==1.3.3,pathlib2==2.3.3,pluggy==0.9.0,-e git+git@github.com:yoavcaspi/pre-commit.git@471fe7d58f99f7276cfe8f77803a8d5227f7c85f#egg=pre_commit,py==1.8.0,pycodestyle==2.5.0,pyflakes==2.1.1,pytest==4.4.1,pytest-env==0.6.2,PyYAML==5.1,scandir==1.10.0,six==1.12.0,toml==0.10.0,typing==3.6.6,virtualenv==16.5.0,zipp==0.4.0
py27 run-test-pre: PYTHONHASHSEED='4088132323'
py27 run-test: commands[0] | coverage erase
py27 run-test: commands[1] | coverage run -m pytest -k test_environment_not_sourced --pdb
============================= test session starts ==============================
platform linux2 -- Python 2.7.15rc1, pytest-4.4.1, py-1.8.0, pluggy-0.9.0
cachedir: .tox/py27/.pytest_cache
rootdir: /home/yocaspi/workspace/pre-commit, inifile: tox.ini
plugins: env-0.6.2
collected 580 items / 579 deselected / 1 selected                              

tests/commands/install_uninstall_test.py F
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> captured stdout >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
pre-commit installed at .git/hooks/pre-commit
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

tempdir_factory = <tests.conftest.TmpdirFactory object at 0x7f1446f6b610>
store = <pre_commit.store.Store object at 0x7f1446f6b710>

    def test_environment_not_sourced(tempdir_factory, store):
        path = make_consuming_repo(tempdir_factory, 'script_hooks_repo')
        with cwd(path):
            # Patch the executable to simulate rming virtualenv
            with mock.patch.object(sys, 'executable', '/does-not-exist'):
                assert install(C.CONFIG_FILE, store) == 0
    
            # Use a specific homedir to ignore --user installs
            homedir = tempdir_factory.get()
            ret, stdout, stderr = git_commit(
                env={
                    'HOME': homedir,
                    'PATH': _path_without_us(),
                    # Git needs this to make a commit
                    'GIT_AUTHOR_NAME': os.environ['GIT_AUTHOR_NAME'],
                    'GIT_COMMITTER_NAME': os.environ['GIT_COMMITTER_NAME'],
                    'GIT_AUTHOR_EMAIL': os.environ['GIT_AUTHOR_EMAIL'],
                    'GIT_COMMITTER_EMAIL': os.environ['GIT_COMMITTER_EMAIL'],
                },
                retcode=None,
            )
>           assert ret == 1
E           assert 0 == 1

tests/commands/install_uninstall_test.py:264: AssertionError
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> /home/yocaspi/workspace/pre-commit/tests/commands/install_uninstall_test.py(264)test_environment_not_sourced()
-> assert ret == 1
(Pdb) _path_without_us()
'/home/yocaspi/workspace/pre-commit/venv-pre_commit/bin:/home/yocaspi/bin:/home/yocaspi/.local/bin:/home/yocaspi/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin'
(Pdb) 

[asottile]

ah yeah this path will have a pre-commit binary in it which the test is discovering: /home/yocaspi/workspace/pre-commit/venv-pre_commit/bin

ideally _path_without_us would strip all path entries that contain a pre-commit executable

[yoavcaspi]

and why do path environment variable is needed in git_commit method?

[asottile]

the .git/hooks/pre-commit scripts makes several attempts to find a suitable pre-commit binary to use (not necessarily in this order):

• which pre-commit
• $installpython -m pre_commit
• python -m pre_commit

This test is demonstrating what happens when all three of those are blown away / unavailable