Re: [PATCH 3/4] t7800: modernize tests

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 17, 2013, 11:21 UTC
Message-ID: <20130217112139.GG6759@elie.Belkin>
In-Reply-To: <1360993666-81308-3-git-send-email-davvid@gmail.com>

David Aguilar wrote:

> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -10,29 +10,11 @@ Testing basic diff tool invocation

[...]

Show 10 quoted lines

> -restore_test_defaults()
> -{
> -	# Restores the test defaults used by several tests
> -	remove_config_vars
> -	unset GIT_DIFF_TOOL
> -	unset GIT_DIFFTOOL_PROMPT
> -	unset GIT_DIFFTOOL_NO_PROMPT
> -	git config diff.tool test-tool &&
> -	git config difftool.test-tool.cmd 'cat $LOCAL'
> -	git config difftool.bogus-tool.cmd false

Yay. :)

[...]

Show 7 quoted lines

>  # Ensures that git-difftool ignores bogus --tool values
>  test_expect_success PERL 'difftool ignores bad --tool values' '
>  	diff=$(git difftool --no-prompt --tool=bad-tool branch)
>  	test "$?" = 1 &&
> -	test "$diff" = ""
> +	test -z "$diff"
>  '

Not about this patch: if I add more commands before that "diff", their exit status would be ignored. Could this be made more resilient using test_expect_code? Something like

	test_expect_code 1 git diff --no-prompt --tool=bad-tool branch >actual &&
	>expect &&
	test_cmp expect actual

[...]

Show 14 quoted lines

>  # Specify the diff tool using $GIT_DIFF_TOOL
>  test_expect_success PERL 'GIT_DIFF_TOOL variable' '
> -	test_might_fail git config --unset diff.tool &&
> +	difftool_test_setup &&
> +	git config --unset diff.tool &&
> +
>  	GIT_DIFF_TOOL=test-tool &&
>  	export GIT_DIFF_TOOL &&
>  
>  	diff=$(git difftool --no-prompt branch) &&
>  	test "$diff" = "branch" &&
> -
> -	restore_test_defaults
> +	sane_unset GIT_DIFF_TOOL

If this test fails, GIT_DIFF_TOOL would remain set which could take down later tests, too. Could it be set in a subprocess (e.g., a subshell) to avoid that?

	difftool_test_setup &&
	git config --unset diff.tool &&

	echo branch >expect &&
	GIT_DIFF_TOOL=test-tool git difftool --no-prompt branch >actual &&
	test_cmp expect actual

[...]

Show 11 quoted lines

>  test_expect_success PERL 'GIT_DIFF_TOOL overrides' '
> -	git config diff.tool bogus-tool &&
> -	git config merge.tool bogus-tool &&
> -
> +	difftool_test_setup &&
> +	test_config diff.tool bogus-tool &&
> +	test_config merge.tool bogus-tool &&
>  	GIT_DIFF_TOOL=test-tool &&
>  	export GIT_DIFF_TOOL &&
>  
>  	diff=$(git difftool --no-prompt branch) &&

Likewise.

[...]

>  	GIT_DIFF_TOOL=bogus-tool &&
>  	export GIT_DIFF_TOOL &&
>  
>  	diff=$(git difftool --no-prompt --tool=test-tool branch) &&

Likewise.

[...]

Show 6 quoted lines

>  test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' '
> +	difftool_test_setup &&
>  	GIT_DIFFTOOL_NO_PROMPT=true &&
>  	export GIT_DIFFTOOL_NO_PROMPT &&
>  
>  	diff=$(git difftool branch) &&

Likewise.

[...]

Show 8 quoted lines

>  test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' '
> -	git config difftool.prompt false &&
> +	difftool_test_setup &&
> +	test_config difftool.prompt false &&
>  	GIT_DIFFTOOL_PROMPT=true &&
>  	export GIT_DIFFTOOL_PROMPT &&
>  
>  	prompt=$(echo | git difftool branch | tail -1) &&

Likewise. This one loses the exit status from 'git difftool', which could be avoided by writing to temporary files:

	echo >input &&
	GIT_DIFFTOOL_PROMPT=true git difftool branch <input >output &&
	prompt=$(tail -1 <output) &&

[...]

Show 8 quoted lines

>  test_expect_success PERL 'difftool last flag wins' '
> +	difftool_test_setup &&
>  	diff=$(git difftool --prompt --no-prompt branch) &&
>  	test "$diff" = "branch" &&
>  
> -	restore_test_defaults &&
> -
>  	prompt=$(echo | git difftool --no-prompt --prompt branch | tail -1) &&

[...]

Likewise.

Thanks for cleaning up, and sorry I don't have anything more substantial to offer.

Hope that helps, Jonathan