Re: [PATCH 6/8] t: fix out-of-tree tests for some git-p4 tests

[Patrick Steinhardt <ps@pks.im>]

On Fri, Dec 13, 2024 at 11:00:23AM +0100, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/t/t9835-git-p4-metadata-encoding-python2.sh b/t/t9835-git-p4-metadata-encoding-python2.sh
> > index 036bf79c6674f6f1f0d667c7270674168428ffee..02f9ec09053890a4d41b7dc95644066d6481bbb6 100755
> > --- a/t/t9835-git-p4-metadata-encoding-python2.sh
> > +++ b/t/t9835-git-p4-metadata-encoding-python2.sh
> > @@ -14,23 +14,25 @@ python_target_version='2'
> >  ## SECTION REPEATED IN t9836 ##
> 
> To be honest, I don't understand why this section wasn't put in a
> function in lib-git-p4.sh in the first place, instead of duplicating?
> Anyhow, I think for two test files it's fine to duplicate this code, and
> after all you're not changing that.
> 
> But I've noticed you are no longer using `python_target_version`. I
> would suggest to either remove the variable, or use it again so the code
> between the two test files is identical again. Doing the latter would
> probably mean you also need to create a variable like
> `p4_python=p4-python$python_target_version` and use `$p4_python` instead
> of `p4-python2` throughout the script, so I'm not sure that improves
> things.

Good catch. I did it in the Python 3 test, but forgot to do it here, as
well.

> >  ###############################
> >  
> > -# Please note: this test calls "git-p4.py" rather than "git-p4", because the
> > -# latter references a specific path so we can't easily force it to run under
> > -# the python version we need to.
> > -
> > -python_major_version=$(python -V 2>&1 | cut -c  8)
> > -python_target_binary=$(which python$python_target_version)
> > -if ! test "$python_major_version" = "$python_target_version" && test "$python_target_binary"
> > +# These tests are specific to Python 2. Write a custom script that executes
> > +# git-p4 directly with the Python 2 interpreter to ensure that we use that
> > +# version even if Git was compiled with Python 3.
> > +python_target_binary=$(which python2)
> > +if test -n "$python_target_binary"
> >  then
> >  	mkdir temp_python
> > -	PATH="$(pwd)/temp_python:$PATH" && export PATH
> > -	ln -s $python_target_binary temp_python/python
> > +	PATH="$(pwd)/temp_python:$PATH"
> > +	export PATH
> > +
> > +	write_script temp_python/git-p4-python2 <<-EOF
> > +	exec "$python_target_binary" "$(git --exec-path)/git-p4" "\$@"
> > +	EOF
> >  fi
> >  
> > -python_major_version=$(python -V 2>&1 | cut -c  8)
> > -if ! test "$python_major_version" = "$python_target_version"
> > +git p4-python2 >err
> > +if ! grep 'valid commands' err
> 
> I like this sanity check, this verifies if the command actually works:
> 
> Thus the output when the script is properly created:
> 
>     usage: /home/toon/devel/git/git-p4 <command> [options]
> 
>     valid commands: branches, clone, sync, submit, unshelve, commit, rebase
> 
>     Try /home/toon/devel/git/git-p4 <command> --help for command specific help.
> 
> 
> And when the script was not written:
> 
>     git: 'p4-python2' is not a git command. See 'git --help'.
> 
> 
> I noticed though, the stderr output isn's shallowed into /dev/null,
> resulting the output for the test to be the following if Python 2 is not found:
> 
>     make[2]: Entering directory '/home/toon/devel/git/t'
>     *** t9835-git-p4-metadata-encoding-python2.sh ***
>     which: no python2 in (/home/toon/devel/git/bin-wrappers:/home/toon/.local/bin:[snip])
>     git: 'p4-python2' is not a git command. See 'git --help'.
>     not ok 1 - start p4d
> 
> 
> I think that's totally fine though, it's giving the user proper
> information about what is wrong.

Yeah, I actually consider it a win to have it. Not that anybody ever
executes these tests outside of CI anyway.

Patrick