X Tutup
Skip to content

change the HOME for git commands#649

Closed
gaborbernat wants to merge 3 commits intopre-commit:masterfrom
gaborbernat:master
Closed

change the HOME for git commands#649
gaborbernat wants to merge 3 commits intopre-commit:masterfrom
gaborbernat:master

Conversation

@gaborbernat
Copy link

this avoids using the users configuration, which can contain e.g. gpg signing with password which would break this tests

@pytest.fixture()
def setup_git(monkeypatch, tmpdir):
# we should not use user configuration
monkeypatch.setenv('HOME', tmpdir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we had the same situation in pre-commit-hooks and I think this is the wrong solution as it has the potential to hide problems with different configurations. We instead opted to pass --no-gpg-sign to any git commits under test: pre-commit/pre-commit-hooks#121

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fixture also doesn't scale (adding it to literally every test) -- perhaps instead an autouse fixture would have been more appropriate

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see

assert _norm_out(ret[1]) == expected
@pytest.fixture(name='test_hook_repo')
def test_hook_repo_fixture(tempdir_factory, store, setup_git):
def do(repo_path,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

skipif_cant_run_bash = pytest.mark.skipif(
parse_shebang.find_executable('bash') is None,
reason='bash isn\'t installed or can\'t be found',
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without bash, most of the testsuite is useless -- pre-commit tests lots of the framework functionality with script hooks as they are fast. I don't think I can accept a PR which adds skips for bash. As bash is necessarily installed on all the platforms we support (it's necessary for git itself to function) this also seems inappropriate.


for path in paths:
if os.path.exists(path):
os.rmdir(path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@gaborbernat
Copy link
Author

@asottile I did not knew bash is required. I'm personally not comfortable knowing this to add this project to tox; as you make it at best cumbersome for people on Windows to develop; now they need to install some kind of bash beside just Python.

@asottile
Copy link
Member

It's not required for runtime, just very important for the test suite. Also if you're running git on windows, you're running bash ;)

@gaborbernat
Copy link
Author

@asottile I'm using cmder which includes bash, however the test fails as it tries to find /usr/bin/bash which is not here; any ideas?

@asottile
Copy link
Member

Can you produce the output of the test as well as which bash and echo $PATH?

@gaborbernat
Copy link
Author

C:\Users\berna\git\tox (speed)
λ env | grep PATH
GIT_LFS_PATH=C:\Program Files\Git LFS
HOMEPATH=\Users\berna
PATH=/c/Program Files/cmder/bin:/c/Program Files/cmder/vendor/conemu-maximus5/ConEmu/Scripts:/c/Program Files/cmder/vendor/conemu-maximus5:/c/Program Files/cmder/vendor/conemu-maximus5/ConEmu:/c/ProgramData/Oracle/Java/javapath:/c/Program Files (x86)/Intel/iCLS Client:/c/Program Files/Intel/iCLS Client:/c/Windows/system32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0:/c/Program Files (x86)/Intel/Intel(R) Management Engine Components/DAL:/c/Program Files/Intel/Intel(R) Management Engine Components/DAL:/c/Program Files (x86)/Intel/Intel(R) Management Engine Components/IPT:/c/Program Files/Intel/Intel(R) Management Engine Components/IPT:/c/Program Files (x86)/NVIDIA Corporation/PhysX/Common:/cmd:/c/WINDOWS/system32:/c/WINDOWS:/c/WINDOWS/System32/Wbem:/c/WINDOWS/System32/WindowsPowerShell/v1.0:/c/Program Files/Git LFS:/c/Program Files/PuTTY:/c/Program Files/nodejs:/c/HashiCorp/Vagrant/bin:/:/c/ProgramData/Anaconda3:/c/ProgramData/Anaconda3/Scripts:/c/Users/berna/AppData/Local/conda/conda/envs:/c/Users/berna/AppData/Local/Microsoft/WindowsApps:/c/Program Files (x86)/Microsoft VS Code/bin:/c/Users/berna/AppData/Roaming/npm:/c/Users/berna/AppData/Local/Google/Cloud SDK/google-cloud-sdk/bin:/usr/bin:/usr/share/vim/vim74:/c/Program Files/cmder
PATHEXT=.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC
VBOX_MSI_INSTALL_PATH=C:\Program Files\Oracle\VirtualBox\

C:\Users\berna\git\tox (speed)
λ which bash
/c/Windows/system32/bash

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

X Tutup