[v5,10/14] t5520: test single-line files by git with test_cmp
diff mbox series

Message ID 2f9052fd94ebb6fe93ea6fe2e7cd3c717635c822.1573517561.git.liu.denton@gmail.com
State New
Headers show
Series
  • t5520: various test cleanup
Related show

Commit Message

Denton Liu Nov. 12, 2019, 12:14 a.m. UTC
In case an invocation of a Git command fails within the subshell, the
failure will be masked. Replace the subshell with a file-redirection and
a call to test_cmp.

This change was done with the following GNU sed expressions:

	s/\(\s*\)test \([^ ]*\) = "$(\(git [^)]*\))"/\1echo \2 >expect \&\&\n\1\3 >actual \&\&\n\1test_cmp expect actual/
	s/\(\s*\)test "$(\(git [^)]*\))" = \([^ ]*\)/\1echo \3 >expect \&\&\n\1\2 >actual \&\&\n\1test_cmp expect actual/

A future patch will clean up situations where we have multiple duplicate
statements within a test case. This is done to keep this patch purely
mechanical.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5520-pull.sh | 64 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 16 deletions(-)

Comments

Junio C Hamano Nov. 12, 2019, 5:17 a.m. UTC | #1
Denton Liu <liu.denton@gmail.com> writes:

> In case an invocation of a Git command fails within the subshell, the
> failure will be masked. Replace the subshell with a file-redirection and
> a call to test_cmp.

I.e.

    test "$(git cmd args)" = "expected-string"

=>

    git cmd args >actual && echo "expected-string" >expect &&
    test_cmp expect actual

which makes sense.  It may break if expected-string begins with a
dash or something silly like that, but a quick eyeballing over the
patch tells me that we are safe there.

Technically, "$(git cmd args)" used as a command line option of
another command is called "command substitution", not "subshell".
The proposed log message may need to be updated.

> This change was done with the following GNU sed expressions:
>
> 	s/\(\s*\)test \([^ ]*\) = "$(\(git [^)]*\))"/\1echo \2 >expect \&\&\n\1\3 >actual \&\&\n\1test_cmp expect actual/
> 	s/\(\s*\)test "$(\(git [^)]*\))" = \([^ ]*\)/\1echo \3 >expect \&\&\n\1\2 >actual \&\&\n\1test_cmp expect actual/
>
> A future patch will clean up situations where we have multiple duplicate
> statements within a test case. This is done to keep this patch purely
> mechanical.

OK.  One thing that worries me is if the existing tests are not
expecting (no pun intended) to see 'expect' or 'actual' (e.g. if
they somehow rely on output of "ls-files -u", we are now adding two
untracked files in the working tree).  Another is if the git command
is expected to produce nothing, possibly after failing, and the test
is expecting to see an empty string---in such a case, the hiding of
the exit status would have been intentional ;-)  We'd want to be sure
that we aren't breaking the tests like that by reading through the
result of applying this patch.

Since this is just a single file, I trust you have already done such
sanity checking ;-)

The mechanical conversion procedure itself looks OK.

Thanks.

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t5520-pull.sh | 64 ++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 16 deletions(-)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 1af6ea06ee..8b7e7ae55d 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -255,7 +255,9 @@ test_expect_success '--rebase' '
>  	git tag before-rebase &&
>  	git pull --rebase . copy &&
>  	test_cmp_rev HEAD^ copy &&
> -	test new = "$(git show HEAD:file2)"
> +	echo new >expect &&
> +	git show HEAD:file2 >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success '--rebase fast forward' '
> @@ -330,7 +332,9 @@ test_expect_success '--rebase fails with multiple branches' '
>  	test_must_fail git pull --rebase . copy master 2>err &&
>  	test_cmp_rev HEAD before-rebase &&
>  	test_i18ngrep "Cannot rebase onto multiple branches" err &&
> -	test modified = "$(git show HEAD:file)"
> +	echo modified >expect &&
> +	git show HEAD:file >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
> @@ -381,7 +385,9 @@ test_expect_success 'pull.rebase' '
>  	test_config pull.rebase true &&
>  	git pull . copy &&
>  	test_cmp_rev HEAD^ copy &&
> -	test new = "$(git show HEAD:file2)"
> +	echo new >expect &&
> +	git show HEAD:file2 >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success 'pull --autostash & pull.rebase=true' '
> @@ -399,7 +405,9 @@ test_expect_success 'branch.to-rebase.rebase' '
>  	test_config branch.to-rebase.rebase true &&
>  	git pull . copy &&
>  	test_cmp_rev HEAD^ copy &&
> -	test new = "$(git show HEAD:file2)"
> +	echo new >expect &&
> +	git show HEAD:file2 >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
> @@ -408,14 +416,18 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
>  	test_config branch.to-rebase.rebase false &&
>  	git pull . copy &&
>  	test_cmp_rev ! HEAD^ copy &&
> -	test new = "$(git show HEAD:file2)"
> +	echo new >expect &&
> +	git show HEAD:file2 >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success 'pull --rebase warns on --verify-signatures' '
>  	git reset --hard before-rebase &&
>  	git pull --rebase --verify-signatures . copy 2>err &&
>  	test_cmp_rev HEAD^ copy &&
> -	test new = "$(git show HEAD:file2)" &&
> +	echo new >expect &&
> +	git show HEAD:file2 >actual &&
> +	test_cmp expect actual &&
>  	test_i18ngrep "ignoring --verify-signatures for rebase" err
>  '
>  
> @@ -423,7 +435,9 @@ test_expect_success 'pull --rebase does not warn on --no-verify-signatures' '
>  	git reset --hard before-rebase &&
>  	git pull --rebase --no-verify-signatures . copy 2>err &&
>  	test_cmp_rev HEAD^ copy &&
> -	test new = "$(git show HEAD:file2)" &&
> +	echo new >expect &&
> +	git show HEAD:file2 >actual &&
> +	test_cmp expect actual &&
>  	test_i18ngrep ! "verify-signatures" err
>  '
>  
> @@ -445,7 +459,9 @@ test_expect_success 'pull.rebase=false create a new merge commit' '
>  	git pull . copy &&
>  	test_cmp_rev HEAD^1 before-preserve-rebase &&
>  	test_cmp_rev HEAD^2 copy &&
> -	test file3 = "$(git show HEAD:file3.t)"
> +	echo file3 >expect &&
> +	git show HEAD:file3.t >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success 'pull.rebase=true flattens keep-merge' '
> @@ -453,7 +469,9 @@ test_expect_success 'pull.rebase=true flattens keep-merge' '
>  	test_config pull.rebase true &&
>  	git pull . copy &&
>  	test_cmp_rev HEAD^^ copy &&
> -	test file3 = "$(git show HEAD:file3.t)"
> +	echo file3 >expect &&
> +	git show HEAD:file3.t >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
> @@ -461,7 +479,9 @@ test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
>  	test_config pull.rebase 1 &&
>  	git pull . copy &&
>  	test_cmp_rev HEAD^^ copy &&
> -	test file3 = "$(git show HEAD:file3.t)"
> +	echo file3 >expect &&
> +	git show HEAD:file3.t >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success REBASE_P \
> @@ -507,7 +527,9 @@ test_expect_success '--rebase=false create a new merge commit' '
>  	git pull --rebase=false . copy &&
>  	test_cmp_rev HEAD^1 before-preserve-rebase &&
>  	test_cmp_rev HEAD^2 copy &&
> -	test file3 = "$(git show HEAD:file3.t)"
> +	echo file3 >expect &&
> +	git show HEAD:file3.t >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success '--rebase=true rebases and flattens keep-merge' '
> @@ -515,7 +537,9 @@ test_expect_success '--rebase=true rebases and flattens keep-merge' '
>  	test_config pull.rebase preserve &&
>  	git pull --rebase=true . copy &&
>  	test_cmp_rev HEAD^^ copy &&
> -	test file3 = "$(git show HEAD:file3.t)"
> +	echo file3 >expect &&
> +	git show HEAD:file3.t >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success REBASE_P \
> @@ -537,7 +561,9 @@ test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-m
>  	test_config pull.rebase preserve &&
>  	git pull --rebase . copy &&
>  	test_cmp_rev HEAD^^ copy &&
> -	test file3 = "$(git show HEAD:file3.t)"
> +	echo file3 >expect &&
> +	git show HEAD:file3.t >actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success '--rebase with rebased upstream' '
> @@ -622,10 +648,16 @@ test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
>  		cd empty_repo2 &&
>  		echo staged-file >staged-file &&
>  		git add staged-file &&
> -		test "$(git ls-files)" = staged-file &&
> +		echo staged-file >expect &&
> +		git ls-files >actual &&
> +		test_cmp expect actual &&
>  		test_must_fail git pull --rebase .. master 2>err &&
> -		test "$(git ls-files)" = staged-file &&
> -		test "$(git show :staged-file)" = staged-file &&
> +		echo staged-file >expect &&
> +		git ls-files >actual &&
> +		test_cmp expect actual &&
> +		echo staged-file >expect &&
> +		git show :staged-file >actual &&
> +		test_cmp expect actual &&
>  		test_i18ngrep "unborn branch with changes added to the index" err
>  	)
>  '
Denton Liu Nov. 12, 2019, 11:06 p.m. UTC | #2
Hi Junio,

On Tue, Nov 12, 2019 at 02:17:33PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > In case an invocation of a Git command fails within the subshell, the
> > failure will be masked. Replace the subshell with a file-redirection and
> > a call to test_cmp.
> 
> I.e.
> 
>     test "$(git cmd args)" = "expected-string"
> 
> =>
> 
>     git cmd args >actual && echo "expected-string" >expect &&
>     test_cmp expect actual
> 
> which makes sense.  It may break if expected-string begins with a
> dash or something silly like that, but a quick eyeballing over the
> patch tells me that we are safe there.
> 
> Technically, "$(git cmd args)" used as a command line option of
> another command is called "command substitution", not "subshell".
> The proposed log message may need to be updated.

Okay, I'll send out a new reroll with some log message cleanup.

> 
> > This change was done with the following GNU sed expressions:
> >
> > 	s/\(\s*\)test \([^ ]*\) = "$(\(git [^)]*\))"/\1echo \2 >expect \&\&\n\1\3 >actual \&\&\n\1test_cmp expect actual/
> > 	s/\(\s*\)test "$(\(git [^)]*\))" = \([^ ]*\)/\1echo \3 >expect \&\&\n\1\2 >actual \&\&\n\1test_cmp expect actual/
> >
> > A future patch will clean up situations where we have multiple duplicate
> > statements within a test case. This is done to keep this patch purely
> > mechanical.
> 
> OK.  One thing that worries me is if the existing tests are not
> expecting (no pun intended) to see 'expect' or 'actual' (e.g. if
> they somehow rely on output of "ls-files -u", we are now adding two
> untracked files in the working tree).

Do you mean `ls-files -o`? I think `-u` means "unmerged" while `-o`
means "others" (or "untracked"). This test doesn't have any instances of
`-o` being used.

> Another is if the git command
> is expected to produce nothing, possibly after failing, and the test
> is expecting to see an empty string---in such a case, the hiding of
> the exit status would have been intentional ;-)  We'd want to be sure
> that we aren't breaking the tests like that by reading through the
> result of applying this patch.
> 
> Since this is just a single file, I trust you have already done such
> sanity checking ;-)

Thanks for double-checking on me. I have, indeed, manually verified the
changes.

> 
> The mechanical conversion procedure itself looks OK.
> 
> Thanks.

Patch
diff mbox series

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 1af6ea06ee..8b7e7ae55d 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -255,7 +255,9 @@  test_expect_success '--rebase' '
 	git tag before-rebase &&
 	git pull --rebase . copy &&
 	test_cmp_rev HEAD^ copy &&
-	test new = "$(git show HEAD:file2)"
+	echo new >expect &&
+	git show HEAD:file2 >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '--rebase fast forward' '
@@ -330,7 +332,9 @@  test_expect_success '--rebase fails with multiple branches' '
 	test_must_fail git pull --rebase . copy master 2>err &&
 	test_cmp_rev HEAD before-rebase &&
 	test_i18ngrep "Cannot rebase onto multiple branches" err &&
-	test modified = "$(git show HEAD:file)"
+	echo modified >expect &&
+	git show HEAD:file >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
@@ -381,7 +385,9 @@  test_expect_success 'pull.rebase' '
 	test_config pull.rebase true &&
 	git pull . copy &&
 	test_cmp_rev HEAD^ copy &&
-	test new = "$(git show HEAD:file2)"
+	echo new >expect &&
+	git show HEAD:file2 >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'pull --autostash & pull.rebase=true' '
@@ -399,7 +405,9 @@  test_expect_success 'branch.to-rebase.rebase' '
 	test_config branch.to-rebase.rebase true &&
 	git pull . copy &&
 	test_cmp_rev HEAD^ copy &&
-	test new = "$(git show HEAD:file2)"
+	echo new >expect &&
+	git show HEAD:file2 >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
@@ -408,14 +416,18 @@  test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 	test_config branch.to-rebase.rebase false &&
 	git pull . copy &&
 	test_cmp_rev ! HEAD^ copy &&
-	test new = "$(git show HEAD:file2)"
+	echo new >expect &&
+	git show HEAD:file2 >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'pull --rebase warns on --verify-signatures' '
 	git reset --hard before-rebase &&
 	git pull --rebase --verify-signatures . copy 2>err &&
 	test_cmp_rev HEAD^ copy &&
-	test new = "$(git show HEAD:file2)" &&
+	echo new >expect &&
+	git show HEAD:file2 >actual &&
+	test_cmp expect actual &&
 	test_i18ngrep "ignoring --verify-signatures for rebase" err
 '
 
@@ -423,7 +435,9 @@  test_expect_success 'pull --rebase does not warn on --no-verify-signatures' '
 	git reset --hard before-rebase &&
 	git pull --rebase --no-verify-signatures . copy 2>err &&
 	test_cmp_rev HEAD^ copy &&
-	test new = "$(git show HEAD:file2)" &&
+	echo new >expect &&
+	git show HEAD:file2 >actual &&
+	test_cmp expect actual &&
 	test_i18ngrep ! "verify-signatures" err
 '
 
@@ -445,7 +459,9 @@  test_expect_success 'pull.rebase=false create a new merge commit' '
 	git pull . copy &&
 	test_cmp_rev HEAD^1 before-preserve-rebase &&
 	test_cmp_rev HEAD^2 copy &&
-	test file3 = "$(git show HEAD:file3.t)"
+	echo file3 >expect &&
+	git show HEAD:file3.t >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'pull.rebase=true flattens keep-merge' '
@@ -453,7 +469,9 @@  test_expect_success 'pull.rebase=true flattens keep-merge' '
 	test_config pull.rebase true &&
 	git pull . copy &&
 	test_cmp_rev HEAD^^ copy &&
-	test file3 = "$(git show HEAD:file3.t)"
+	echo file3 >expect &&
+	git show HEAD:file3.t >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
@@ -461,7 +479,9 @@  test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
 	test_config pull.rebase 1 &&
 	git pull . copy &&
 	test_cmp_rev HEAD^^ copy &&
-	test file3 = "$(git show HEAD:file3.t)"
+	echo file3 >expect &&
+	git show HEAD:file3.t >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success REBASE_P \
@@ -507,7 +527,9 @@  test_expect_success '--rebase=false create a new merge commit' '
 	git pull --rebase=false . copy &&
 	test_cmp_rev HEAD^1 before-preserve-rebase &&
 	test_cmp_rev HEAD^2 copy &&
-	test file3 = "$(git show HEAD:file3.t)"
+	echo file3 >expect &&
+	git show HEAD:file3.t >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '--rebase=true rebases and flattens keep-merge' '
@@ -515,7 +537,9 @@  test_expect_success '--rebase=true rebases and flattens keep-merge' '
 	test_config pull.rebase preserve &&
 	git pull --rebase=true . copy &&
 	test_cmp_rev HEAD^^ copy &&
-	test file3 = "$(git show HEAD:file3.t)"
+	echo file3 >expect &&
+	git show HEAD:file3.t >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success REBASE_P \
@@ -537,7 +561,9 @@  test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-m
 	test_config pull.rebase preserve &&
 	git pull --rebase . copy &&
 	test_cmp_rev HEAD^^ copy &&
-	test file3 = "$(git show HEAD:file3.t)"
+	echo file3 >expect &&
+	git show HEAD:file3.t >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '--rebase with rebased upstream' '
@@ -622,10 +648,16 @@  test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
 		cd empty_repo2 &&
 		echo staged-file >staged-file &&
 		git add staged-file &&
-		test "$(git ls-files)" = staged-file &&
+		echo staged-file >expect &&
+		git ls-files >actual &&
+		test_cmp expect actual &&
 		test_must_fail git pull --rebase .. master 2>err &&
-		test "$(git ls-files)" = staged-file &&
-		test "$(git show :staged-file)" = staged-file &&
+		echo staged-file >expect &&
+		git ls-files >actual &&
+		test_cmp expect actual &&
+		echo staged-file >expect &&
+		git show :staged-file >actual &&
+		test_cmp expect actual &&
 		test_i18ngrep "unborn branch with changes added to the index" err
 	)
 '