Re: [PATCH 11/14] test: completion: tests for __gitcomp regression

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Fri, Jun 21, 2019 at 05:31:04PM -0500, Felipe Contreras wrote:
> There's a regression in the completion since the introduction of
> __gitcomp.
> 
> Go to any directory that doesn't contain a git repository, like /tmp.
> Then type the following:
> 
>   git checkout --<tab>
> 
> You will see nothing. That's because
> `git checkout --git-completion-helper` fails when you run it outside a
> git repository.
> 
> You might change to a directory that has a git repository, but it's too
> late, because the empty options have been cached.

This will get outdated rather soonish, as soon as 69702523af
(completion: do not cache if --git-completion-helper fails,
2019-06-12) graduates to master.

> It's unclear how many commands are affected, but this patch attempts to
> at least detect some already in the testing framework.

It seems that several changes in this patch modify tests in a way that
defeats the purpose of the given test, e.g. the tests
'completion.commands removes multiple commands' or 'sourcing the
completion script clears cached merge strategies'

I would rather see tests specifically focusing on the
__gitcomp_builtin() helper function, including test cases when it's
excersized outside of a repository and when it gets additional
parameters to include and exclude some options.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t9902-completion.sh | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 43cf313a1c..7bef41eaf5 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -122,6 +122,15 @@ test_gitcomp_nl ()
>  	test_cmp expected out
>  }
>  
> +offgit ()
> +{
> +	GIT_CEILING_DIRECTORIES="$ROOT" &&
> +	export GIT_CEILING_DIRECTORIES &&
> +	test_when_finished "ROOT='$ROOT'; cd '$TRASH_DIRECTORY'; unset GIT_CEILING_DIRECTORIES" &&
> +	ROOT="$ROOT"/non-repo &&
> +	cd "$ROOT"

I think fiddling with $ROOT is unnecessary here.

> +}
> +
>  invalid_variable_name='${foo.bar}'
>  
>  actual="$TRASH_DIRECTORY/actual"
> @@ -358,10 +367,8 @@ test_expect_success SYMLINKS '__git_find_repo_path - resulting path avoids symli
>  '
>  
>  test_expect_success '__git_find_repo_path - not a git repository' '
> +	offgit &&
>  	(
> -		cd non-repo &&
> -		GIT_CEILING_DIRECTORIES="$ROOT" &&
> -		export GIT_CEILING_DIRECTORIES &&
>  		test_must_fail __git_find_repo_path &&
>  		printf "$__git_repo_path" >"$actual"
>  	) &&
> @@ -1388,6 +1395,7 @@ test_expect_success '__git_pretty_aliases' '
>  '
>  
>  test_expect_success 'basic' '
> +	offgit &&
>  	run_completion "git " &&
>  	# built-in
>  	grep -q "^add \$" out &&
> @@ -1401,6 +1409,7 @@ test_expect_success 'basic' '
>  '
>  
>  test_expect_success 'double dash "git" itself' '
> +	offgit &&
>  	test_completion "git --" <<-\EOF
>  	--paginate Z
>  	--no-pager Z
> @@ -1419,7 +1428,8 @@ test_expect_success 'double dash "git" itself' '
>  	EOF
>  '
>  
> -test_expect_success 'double dash "git checkout"' '
> +test_expect_failure 'double dash "git checkout"' '
> +	offgit &&
>  	test_completion "git checkout --" <<-\EOF
>  	--quiet Z
>  	--detach Z
> @@ -1442,6 +1452,7 @@ test_expect_success 'double dash "git checkout"' '
>  '
>  
>  test_expect_success 'general options' '
> +	offgit &&
>  	test_completion "git --ver" "--version " &&
>  	test_completion "git --hel" "--help " &&
>  	test_completion "git --exe" <<-\EOF &&
> @@ -1460,6 +1471,7 @@ test_expect_success 'general options' '
>  '
>  
>  test_expect_success 'general options plus command' '
> +	offgit &&
>  	test_completion "git --version check" "checkout " &&
>  	test_completion "git --paginate check" "checkout " &&
>  	test_completion "git --git-dir=foo check" "checkout " &&
> @@ -1480,11 +1492,13 @@ test_expect_success 'general options plus command' '
>  '
>  
>  test_expect_success 'git --help completion' '
> +	offgit &&
>  	test_completion "git --help ad" "add " &&
>  	test_completion "git --help core" "core-tutorial "
>  '
>  
> -test_expect_success 'completion.commands removes multiple commands' '
> +test_expect_failure 'completion.commands removes multiple commands' '
> +	offgit &&
>  	test_config completion.commands "-cherry -mergetool" &&
>  	git --list-cmds=list-mainporcelain,list-complete,config >out &&
>  	! grep -E "^(cherry|mergetool)$" out
> @@ -1547,9 +1561,10 @@ test_expect_success 'complete tree filename with metacharacters' '
>  	EOF
>  '
>  
> -test_expect_success PERL 'send-email' '
> -	test_completion "git send-email --cov" "--cover-letter " &&
> -	test_completion "git send-email ma" "master "
> +test_expect_failure PERL 'send-email' '
> +	test_completion "git send-email ma" "master " &&
> +	offgit &&
> +	test_completion "git send-email --cov" "--cover-letter "
>  '
>  
>  test_expect_success 'complete files' '
> @@ -1649,6 +1664,7 @@ test_expect_success 'completion used <cmd> completion for alias: !f() { : git <c
>  '
>  
>  test_expect_success 'completion without explicit _git_xxx function' '
> +	offgit &&
>  	test_completion "git version --" <<-\EOF
>  	--build-options Z
>  	--no-build-options Z
> @@ -1699,13 +1715,15 @@ do
>  done
>  
>  test_expect_success 'sourcing the completion script clears cached commands' '
> +	offgit &&
>  	__git_compute_all_commands &&
>  	verbose test -n "$__git_all_commands" &&
>  	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
>  	verbose test -z "$__git_all_commands"
>  '
>  
> -test_expect_success 'sourcing the completion script clears cached merge strategies' '
> +test_expect_failure 'sourcing the completion script clears cached merge strategies' '
> +	offgit &&
>  	GIT_TEST_GETTEXT_POISON= &&
>  	__git_compute_merge_strategies &&
>  	verbose test -n "$__git_merge_strategies" &&
> @@ -1714,6 +1732,7 @@ test_expect_success 'sourcing the completion script clears cached merge strategi
>  '
>  
>  test_expect_success 'sourcing the completion script clears cached --options' '
> +	offgit &&
>  	__gitcomp_builtin checkout &&
>  	verbose test -n "$__gitcomp_builtin_checkout" &&
>  	__gitcomp_builtin notes_edit &&
> -- 
> 2.22.0
>