diff mbox series

[v2] t7001-mv.sh: modernizing test script using functions

Message ID pull.1372.v2.git.git.1667500788132.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2] t7001-mv.sh: modernizing test script using functions | expand

Commit Message

Debra Obondo Nov. 3, 2022, 6:39 p.m. UTC 
From: Debra Obondo <debraobondo@gmail.com>

Test script to verify the presence/absence of files, paths, directories,
symlinks and other features in 'git mv' command are using the command
format:

'test (-e|f|d|h|...)'

Replace them with helper functions of format:

'test_path_is_*'

Changes since v1

Replacing idiomatic helper functions

'! test_path_is_*'

with

'test_path_is_missing'

This uses values of 'test_path_bar' in place of '! test_path_foo' to
bring in the helpful factor of indicating the failure of tests after the
mv command has been used, that is, it echoes if the feature/test_path
exists.

Signed-off-by: Debra Obondo <debraobondo@gmail.com>
---
    [PATCH v2] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in
    test script
    
    Changes since v1:
    
    Replacing idiomatic helper functions
    
    '! test_path_is_*'
    
    with
    
    'test_path_is_missing'
    
    This uses values of 'test_path_bar' in place of '! test_path_foo' to
    bring in the helpful factor of indicating the failure of tests after the
    mv command has been used, that is, it echoes if the feature/test_path
    exists (This was suggested by Martin Ågren).
    
    Older test scripts use the command 'test -' to verify the presence or
    absence of features such as files, directories and symbolic links. This
    however requires slightly complicated uses, such as 'test ! -f '. The
    helper functions 'test_path_is_' located in t/test-lib-functions.sh have
    taken into account all scenarios of the 'test -*' to reduce errors. This
    was a microproject to replace them with the helper functions.
    
    Test script to verify the presence/absence of files, paths,
    directories,symboliclinks and many other features in mv command are
    using the command format
    
    'test -(e|f|s|h|...).
    
    Replace them with helper functions of format
    
    'test_path_is_*'
    
    Signed-off-by: Debra Obondo debraobondo@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1372%2Ffobiasic07%2Ft7001-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1372/fobiasic07/t7001-v2
Pull-Request: https://github.com/git/git/pull/1372

Range-diff vs v1:

 1:  b977c4ad26a ! 1:  c6640ebff66 t7001-mv.sh:modernizing test script using function
     @@ Metadata
      Author: Debra Obondo <debraobondo@gmail.com>
      
       ## Commit message ##
     -    t7001-mv.sh:modernizing test script using function
     +    t7001-mv.sh: modernizing test script using functions
      
          Test script to verify the presence/absence of files, paths, directories,
          symlinks and other features in 'git mv' command are using the command
     @@ Commit message
      
          'test_path_is_*'
      
     +    Changes since v1
     +
     +    Replacing idiomatic helper functions
     +
     +    '! test_path_is_*'
     +
     +    with
     +
     +    'test_path_is_missing'
     +
     +    This uses values of 'test_path_bar' in place of '! test_path_foo' to
     +    bring in the helpful factor of indicating the failure of tests after the
     +    mv command has been used, that is, it echoes if the feature/test_path
     +    exists.
     +
          Signed-off-by: Debra Obondo <debraobondo@gmail.com>
      
       ## t/t7001-mv.sh ##
     @@ t/t7001-mv.sh: test_expect_success 'checking the commit' '
      -	test -f path0/COPYING &&
      -	test ! -f MOVED
      +	test_path_is_file path0/COPYING &&
     -+	! test_path_is_file MOVED
     ++	test_path_is_missing MOVED
       '
       
       test_expect_success 'checking -k on non-existing file' '
     @@ t/t7001-mv.sh: test_expect_success 'checking -k on non-existing file' '
      -	test -f untracked1 &&
      -	test ! -f path0/untracked1
      +	test_path_is_file untracked1 &&
     -+	! test_path_is_file path0/untracked1
     ++	test_path_is_missing path0/untracked1
       '
       
       test_expect_success 'checking -k on multiple untracked files' '
     @@ t/t7001-mv.sh: test_expect_success 'checking -k on non-existing file' '
      -	test ! -f path0/untracked2
      +	test_path_is_file untracked1 &&
      +	test_path_is_file untracked2 &&
     -+	! test_path_is_file path0/untracked1 &&
     -+	! test_path_is_file path0/untracked2
     ++	test_path_is_missing path0/untracked1 &&
     ++	test_path_is_missing path0/untracked2
       '
       
       test_expect_success 'checking -f on untracked file with existing target' '
     @@ t/t7001-mv.sh: test_expect_success 'checking -k on non-existing file' '
      -	test ! -f .git/index.lock &&
      -	test -f untracked1 &&
      -	test -f path0/untracked1
     -+	! test_path_is_file .git/index.lock &&
     ++	test_path_is_missing .git/index.lock &&
      +	test_path_is_file untracked1 &&
      +	test_path_is_file path0/untracked1
       '
     @@ t/t7001-mv.sh: test_expect_success 'absolute pathname' '
       		git mv sub "$(pwd)/in" &&
      -		! test -d sub &&
      -		test -d in &&
     -+		! test_path_is_dir sub &&
     ++		test_path_is_missing sub &&
      +		test_path_is_dir in &&
       		git ls-files --error-unmatch in/file
       	)
     @@ t/t7001-mv.sh: test_expect_success 'absolute pathname outside should fail' '
      -		test -d sub &&
      -		! test -d ../in &&
      +		test_path_is_dir sub &&
     -+		! test_path_is_dir ../in &&
     ++		test_path_is_missing ../in &&
       		git ls-files --error-unmatch sub/file
       	)
       '
     @@ t/t7001-mv.sh: test_expect_success 'git mv should overwrite symlink to a file' '
       	git mv -f moved symlink &&
      -	! test -e moved &&
      -	test -f symlink &&
     -+	! test_path_exists moved &&
     ++	test_path_is_missing moved &&
      +	test_path_is_file symlink &&
       	test "$(cat symlink)" = 1 &&
       	git update-index --refresh &&
     @@ t/t7001-mv.sh: test_expect_success 'git mv should overwrite file with a symlink'
       	test_must_fail git mv symlink moved &&
       	git mv -f symlink moved &&
      -	! test -e symlink &&
     -+	! test_path_exists symlink &&
     ++	test_path_is_missing symlink &&
       	git update-index --refresh &&
       	git diff-files --quiet
       '
     @@ t/t7001-mv.sh: test_expect_success 'git mv moves a submodule with a .git directo
       	mkdir mod &&
       	git mv sub mod/sub &&
      -	! test -e sub &&
     -+	! test_path_exists sub &&
     ++	test_path_is_missing sub &&
       	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
       	git -C mod/sub status &&
       	git update-index --refresh &&
     @@ t/t7001-mv.sh: test_expect_success 'git mv moves a submodule with a .git directo
       	mkdir mod &&
       	git mv sub mod/sub &&
      -	! test -e sub &&
     -+	! test_path_exists sub &&
     ++	test_path_is_missing sub &&
       	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
       	git -C mod/sub status &&
       	echo mod/sub >expected &&
     @@ t/t7001-mv.sh: test_expect_success 'git mv moves a submodule with gitfile' '
       	mkdir mod &&
       	git -C mod mv ../sub/ . &&
      -	! test -e sub &&
     -+	! test_path_exists sub &&
     ++	test_path_is_missing sub &&
       	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
       	git -C mod/sub status &&
       	echo mod/sub >expected &&
     @@ t/t7001-mv.sh: test_expect_success 'mv does not complain when no .gitmodules fil
       	git mv sub mod/sub 2>actual.err &&
       	test_must_be_empty actual.err &&
      -	! test -e sub &&
     -+	! test_path_exists sub &&
     ++	test_path_is_missing sub &&
       	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
       	git -C mod/sub status &&
       	git update-index --refresh &&
     @@ t/t7001-mv.sh: test_expect_success 'mv will error out on a modified .gitmodules
       	git mv sub mod/sub 2>actual.err &&
       	test_must_be_empty actual.err &&
      -	! test -e sub &&
     -+	! test_path_exists sub &&
     ++	test_path_is_missing sub &&
       	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
       	git -C mod/sub status &&
       	git update-index --refresh &&
     @@ t/t7001-mv.sh: test_expect_success 'mv issues a warning when section is not foun
       	git mv sub mod/sub 2>actual.err &&
       	test_cmp expect.err actual.err &&
      -	! test -e sub &&
     -+	! test_path_exists sub &&
     ++	test_path_is_missing sub &&
       	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
       	git -C mod/sub status &&
       	git update-index --refresh &&
     @@ t/t7001-mv.sh: test_expect_success 'checking out a commit before submodule moved
       	test_cmp expected actual &&
      -	! test -f sub/.git &&
      -	test -f sub2/.git &&
     -+	! test_path_is_file sub/.git &&
     ++	test_path_is_missing sub/.git &&
      +	test_path_is_file sub2/.git &&
       	git submodule update &&
      -	test -f sub/.git &&


 t/t7001-mv.sh | 62 +++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)


base-commit: 1fc3c0ad407008c2f71dd9ae1241d8b75f8ef886

Comments

Martin Ågren Nov. 3, 2022, 7:37 p.m. UTC | #1 
On Thu, 3 Nov 2022 at 19:39, Debra Obondo via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Debra Obondo <debraobondo@gmail.com>
>
> Test script to verify the presence/absence of files, paths, directories,
> symlinks and other features in 'git mv' command are using the command
> format:
>
> 'test (-e|f|d|h|...)'
>
> Replace them with helper functions of format:
>
> 'test_path_is_*'

Ok.

> Changes since v1
>
> Replacing idiomatic helper functions
>
> '! test_path_is_*'
>
> with
>
> 'test_path_is_missing'
>

The above "Changes since v1" and what I've quoted here should probably
be dropped. We prefer our patches to look as if they've appeared out of
the blue in perfect shape. :-)

> This uses values of 'test_path_bar' in place of '! test_path_foo' to
> bring in the helpful factor of indicating the failure of tests after the
> mv command has been used, that is, it echoes if the feature/test_path
> exists.

This paragraph is excellent. It describes why you've done the patch this
way. This looks much better than version 1 of the patch!

> Signed-off-by: Debra Obondo <debraobondo@gmail.com>

After removing the lines about changes since v1, this patch is

Reviewed-by: Martin Ågren <martin.agren@gmail.com>

> ---
>     [PATCH v2] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in
>     test script
>
>     Changes since v1:
>
>     Replacing idiomatic helper functions
>
>     '! test_path_is_*'
>
>     with
>
>     'test_path_is_missing'

This place after the "---" line is an excellent place for including such
"here's what has changed since v1" comments. Good. They will not appear
in the final commit message.

Thanks for contributing!

Martin
diff mbox series

Patch

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 8c37bceb336..d72cef88264 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -60,8 +60,8 @@  test_expect_success 'checking the commit' '
 
 test_expect_success 'mv --dry-run does not move file' '
 	git mv -n path0/COPYING MOVED &&
-	test -f path0/COPYING &&
-	test ! -f MOVED
+	test_path_is_file path0/COPYING &&
+	test_path_is_missing MOVED
 '
 
 test_expect_success 'checking -k on non-existing file' '
@@ -71,25 +71,25 @@  test_expect_success 'checking -k on non-existing file' '
 test_expect_success 'checking -k on untracked file' '
 	>untracked1 &&
 	git mv -k untracked1 path0 &&
-	test -f untracked1 &&
-	test ! -f path0/untracked1
+	test_path_is_file untracked1 &&
+	test_path_is_missing path0/untracked1
 '
 
 test_expect_success 'checking -k on multiple untracked files' '
 	>untracked2 &&
 	git mv -k untracked1 untracked2 path0 &&
-	test -f untracked1 &&
-	test -f untracked2 &&
-	test ! -f path0/untracked1 &&
-	test ! -f path0/untracked2
+	test_path_is_file untracked1 &&
+	test_path_is_file untracked2 &&
+	test_path_is_missing path0/untracked1 &&
+	test_path_is_missing path0/untracked2
 '
 
 test_expect_success 'checking -f on untracked file with existing target' '
 	>path0/untracked1 &&
 	test_must_fail git mv -f untracked1 path0 &&
-	test ! -f .git/index.lock &&
-	test -f untracked1 &&
-	test -f path0/untracked1
+	test_path_is_missing .git/index.lock &&
+	test_path_is_file untracked1 &&
+	test_path_is_file path0/untracked1
 '
 
 # clean up the mess in case bad things happen
@@ -215,8 +215,8 @@  test_expect_success 'absolute pathname' '
 		git add sub/file &&
 
 		git mv sub "$(pwd)/in" &&
-		! test -d sub &&
-		test -d in &&
+		test_path_is_missing sub &&
+		test_path_is_dir in &&
 		git ls-files --error-unmatch in/file
 	)
 '
@@ -234,8 +234,8 @@  test_expect_success 'absolute pathname outside should fail' '
 		git add sub/file &&
 
 		test_must_fail git mv sub "$out/out" &&
-		test -d sub &&
-		! test -d ../in &&
+		test_path_is_dir sub &&
+		test_path_is_missing ../in &&
 		git ls-files --error-unmatch sub/file
 	)
 '
@@ -295,8 +295,8 @@  test_expect_success 'git mv should overwrite symlink to a file' '
 	git add moved &&
 	test_must_fail git mv moved symlink &&
 	git mv -f moved symlink &&
-	! test -e moved &&
-	test -f symlink &&
+	test_path_is_missing moved &&
+	test_path_is_file symlink &&
 	test "$(cat symlink)" = 1 &&
 	git update-index --refresh &&
 	git diff-files --quiet
@@ -312,13 +312,13 @@  test_expect_success 'git mv should overwrite file with a symlink' '
 	git add moved &&
 	test_must_fail git mv symlink moved &&
 	git mv -f symlink moved &&
-	! test -e symlink &&
+	test_path_is_missing symlink &&
 	git update-index --refresh &&
 	git diff-files --quiet
 '
 
 test_expect_success SYMLINKS 'check moved symlink' '
-	test -h moved
+	test_path_is_symlink moved
 '
 
 rm -f moved symlink
@@ -352,7 +352,7 @@  test_expect_success 'git mv moves a submodule with a .git directory and no .gitm
 	) &&
 	mkdir mod &&
 	git mv sub mod/sub &&
-	! test -e sub &&
+	test_path_is_missing sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
@@ -372,7 +372,7 @@  test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu
 	) &&
 	mkdir mod &&
 	git mv sub mod/sub &&
-	! test -e sub &&
+	test_path_is_missing sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	echo mod/sub >expected &&
@@ -389,7 +389,7 @@  test_expect_success 'git mv moves a submodule with gitfile' '
 	entry="$(git ls-files --stage sub | cut -f 1)" &&
 	mkdir mod &&
 	git -C mod mv ../sub/ . &&
-	! test -e sub &&
+	test_path_is_missing sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	echo mod/sub >expected &&
@@ -408,7 +408,7 @@  test_expect_success 'mv does not complain when no .gitmodules file is found' '
 	mkdir mod &&
 	git mv sub mod/sub 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -e sub &&
+	test_path_is_missing sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
@@ -423,13 +423,13 @@  test_expect_success 'mv will error out on a modified .gitmodules file unless sta
 	entry="$(git ls-files --stage sub | cut -f 1)" &&
 	mkdir mod &&
 	test_must_fail git mv sub mod/sub 2>actual.err &&
-	test -s actual.err &&
-	test -e sub &&
+	test_file_not_empty actual.err &&
+	test_path_exists sub &&
 	git diff-files --quiet -- sub &&
 	git add .gitmodules &&
 	git mv sub mod/sub 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -e sub &&
+	test_path_is_missing sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
@@ -447,7 +447,7 @@  test_expect_success 'mv issues a warning when section is not found in .gitmodule
 	mkdir mod &&
 	git mv sub mod/sub 2>actual.err &&
 	test_cmp expect.err actual.err &&
-	! test -e sub &&
+	test_path_is_missing sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
@@ -460,7 +460,7 @@  test_expect_success 'mv --dry-run does not touch the submodule or .gitmodules' '
 	git submodule update &&
 	mkdir mod &&
 	git mv -n sub mod/sub 2>actual.err &&
-	test -f sub/.git &&
+	test_path_is_file sub/.git &&
 	git diff-index --exit-code HEAD &&
 	git update-index --refresh &&
 	git diff-files --quiet -- sub .gitmodules
@@ -474,10 +474,10 @@  test_expect_success 'checking out a commit before submodule moved needs manual u
 	git status -s sub2 >actual &&
 	echo "?? sub2/" >expected &&
 	test_cmp expected actual &&
-	! test -f sub/.git &&
-	test -f sub2/.git &&
+	test_path_is_missing sub/.git &&
+	test_path_is_file sub2/.git &&
 	git submodule update &&
-	test -f sub/.git &&
+	test_path_is_file sub/.git &&
 	rm -rf sub2 &&
 	git diff-index --exit-code HEAD &&
 	git update-index --refresh &&