| Message ID | 20250303141800.12848-1-ingleprachit101@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GSOC] Modernize Test Path Checking in Git’s Test Suite | expand |
On Mon, Mar 3, 2025 at 7:49 PM Prachit Ingle <ingleprachit101@gmail.com> wrote: > > This patch improves the Git test suite by converting old-style path checks to use > modern Git test helpers. Specifically, we have replaced shell commands like `test -f` > and `test -d` with the appropriate Git test helpers, such as `test_path_is_file` and > `test_path_is_dir`. This enhances the readability and consistency of the test suite. > > The following tests were updated: > - t/chainlint/cuddled-loop.test > - t/chainlint/cuddled.test > - t/chainlint/double-here-doc.test > - t/chainlint/dqstring-line-splice.test > - t/chainlint/dqstring-no-interpolate.test > - t/chainlint/empty-here-doc.test > - t/chainlint/exclamation.test > - t/chainlint/exit-loop.test > - t/chainlint/exit-subshell.test > - t/chainlint/for-loop-abbreviated.test > - t/chainlint/for-loop.test > - t/chainlint/function.expect > - t/chainlint/function.test > - t/chainlint/here-doc-body-indent.test > > The changes have been verified by running the test suite to ensure no breaks or regressions. > > Command used to find instances: git grep 'test -[efd]' t/ > > Signed-off-by: Prachit Ingle <ingleprachit101@gmail.com> > --- > t/chainlint/chained-subshell.expect | 2 +- > t/chainlint/chained-subshell.test | 30 ++++++++++++++--------------- > t/chainlint/function.expect | 2 +- > t/chainlint/function.test | 30 ++++++++++++++--------------- > t/interop/interop-lib.sh | 4 ++-- > t/lib-httpd/apply-one-time-perl.sh | 2 +- > t/lib-httpd/nph-custom-auth.sh | 2 +- > t/perf/p5302-pack-index.sh | 2 +- > t/perf/p7527-builtin-fsmonitor.sh | 2 +- > t/perf/perf-lib.sh | 2 +- > 10 files changed, 39 insertions(+), 39 deletions(-) > > diff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect > index 93fb1a6578..49efa1301d 100644 > --- a/t/chainlint/chained-subshell.expect > +++ b/t/chainlint/chained-subshell.expect > @@ -5,6 +5,6 @@ > 6 ) && > 7 > 8 cut "-d " -f actual | (read s1 s2 s3 && > -9 test -f $s1 ?!LINT: missing '&&'?! > +9 test_path_is_file "$s1" || error "$s1 should exist" > 10 test $(cat $s2) = tree2path1 && > 11 test $(cat $s3) = tree3path1) > diff --git a/t/chainlint/chained-subshell.test b/t/chainlint/chained-subshell.test > index 1f11f65398..e167335f1e 100644 > --- a/t/chainlint/chained-subshell.test > +++ b/t/chainlint/chained-subshell.test > @@ -1,15 +1,15 @@ > -test_expect_success 'chained-subshell' ' > -# LINT: start of subshell chained to preceding command > -mkdir sub && ( > - cd sub && > - foo the bar > - nuff said > -) && > - > -# LINT: preceding command pipes to subshell on same line > -cut "-d " -f actual | (read s1 s2 s3 && > -test -f $s1 > -test $(cat $s2) = tree2path1 && > -# LINT: closing subshell ")" correctly detected on same line as "$(...)" > -test $(cat $s3) = tree3path1) > -' > +test_expect_success 'chained-subshell' ' > +# LINT: start of subshell chained to preceding command > +mkdir sub && ( > + cd sub && > + foo the bar > + nuff said > +) && > + > +# LINT: preceding command pipes to subshell on same line > +cut "-d " -f actual | (read s1 s2 s3 && > +test_path_is_file $s1 > +test $(cat $s2) = tree2path1 && > +# LINT: closing subshell ")" correctly detected on same line as "$(...)" > +test $(cat $s3) = tree3path1) > +' > diff --git a/t/chainlint/function.expect b/t/chainlint/function.expect > index 9e46a3554a..a0a465e6d4 100644 > --- a/t/chainlint/function.expect > +++ b/t/chainlint/function.expect > @@ -4,7 +4,7 @@ > 5 > 6 remove_object() { > 7 file=$(sha1_file "$*") && > -8 test -e "$file" ?!LINT: missing '&&'?! > +8 test_path_exists "$file" || error "$file should exist" > 9 rm -f "$file" > 10 } ?!LINT: missing '&&'?! > 11 > diff --git a/t/chainlint/function.test b/t/chainlint/function.test > index 763fcf3f87..841c60720c 100644 > --- a/t/chainlint/function.test > +++ b/t/chainlint/function.test > @@ -1,15 +1,15 @@ > -test_expect_success 'function' ' > -# LINT: "()" in function definition not mistaken for subshell > -sha1_file() { > - echo "$*" | sed "s#..#.git/objects/&/#" > -} && > - > -# LINT: broken &&-chain in function and after function > -remove_object() { > - file=$(sha1_file "$*") && > - test -e "$file" > - rm -f "$file" > -} > - > -sha1_file arg && remove_object arg > -' > +test_expect_success 'function' ' > +# LINT: "()" in function definition not mistaken for subshell > +sha1_file() { > + echo "$*" | sed "s#..#.git/objects/&/#" > +} && > + > +# LINT: broken &&-chain in function and after function > +remove_object() { > + file=$(sha1_file "$*") && > + test_path_exists "$file" || error "$file should exist" > + rm -f "$file" > +} > + > +sha1_file arg && remove_object arg > +' > diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh > index 1b5864d2a7..b2055d4252 100644 > --- a/t/interop/interop-lib.sh > +++ b/t/interop/interop-lib.sh > @@ -21,7 +21,7 @@ build_version () { > sha1=$(git rev-parse "$1^{tree}") || return 1 > dir=$BUILD_ROOT/$sha1 > > - if test -e "$dir/.built" > + if test_path_exists "$dir/.built" > then > echo "$dir" > return 0 > @@ -37,7 +37,7 @@ build_version () { > > for config in config.mak config.mak.autogen config.status > do > - if test -e "$INTEROP_ROOT/../../$config" > + if test_path_exists "$INTEROP_ROOT/../../$config" > then > cp "$INTEROP_ROOT/../../$config" "$dir/" || return 1 > fi > diff --git a/t/lib-httpd/apply-one-time-perl.sh b/t/lib-httpd/apply-one-time-perl.sh > index d7f9fed6ae..83ede36efb 100644 > --- a/t/lib-httpd/apply-one-time-perl.sh > +++ b/t/lib-httpd/apply-one-time-perl.sh > @@ -7,7 +7,7 @@ > # > # This can be used to simulate the effects of the repository changing in > # between HTTP request-response pairs. > -if test -f one-time-perl > +if test_path_is_file one-time-perl > then > LC_ALL=C > export LC_ALL > diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh > index d408d2caad..c39b816c45 100644 > --- a/t/lib-httpd/nph-custom-auth.sh > +++ b/t/lib-httpd/nph-custom-auth.sh > @@ -41,7 +41,7 @@ then > fi > > echo 'HTTP/1.1 401 Authorization Required' > -if test -f "$CHALLENGE_FILE" > +if test_path_is_file "$CHALLENGE_FILE" > then > sed -ne 's/^id=default.*response=//p' "$CHALLENGE_FILE" > fi > diff --git a/t/perf/p5302-pack-index.sh b/t/perf/p5302-pack-index.sh > index 14c601bbf8..d3a3ea360f 100755 > --- a/t/perf/p5302-pack-index.sh > +++ b/t/perf/p5302-pack-index.sh > @@ -9,7 +9,7 @@ test_perf_large_repo > test_expect_success 'repack' ' > git repack -ad && > PACK=$(ls .git/objects/pack/*.pack | head -n1) && > - test -f "$PACK" && > + test_path_is_file "$PACK" && > export PACK > ' > > diff --git a/t/perf/p7527-builtin-fsmonitor.sh b/t/perf/p7527-builtin-fsmonitor.sh > index 90164327e8..16e56cc197 100755 > --- a/t/perf/p7527-builtin-fsmonitor.sh > +++ b/t/perf/p7527-builtin-fsmonitor.sh > @@ -46,7 +46,7 @@ export TMP_BR > REPO=../repos/gen-many-files-"$PARAMS".git > export REPO > > -if ! test -d $REPO > +if ! test_path_is_dir $REPO > then > (cd ../repos; ./many-files.sh -d $PARAM_D -w $PARAM_W -f $PARAM_F) > fi > diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh > index 8ab6d9c469..13eb33ae6f 100644 > --- a/t/perf/perf-lib.sh > +++ b/t/perf/perf-lib.sh > @@ -117,7 +117,7 @@ test_perf_create_repo_from () { > "$MODERN_GIT" init -q && > test_perf_do_repo_symlink_config_ && > mv .git/hooks .git/hooks-disabled 2>/dev/null && > - if test -f .git/index.lock > + if test_path_is_file .git/index.lock > then > # We may be copying a repo that can't run "git > # status" due to a locked index. Since we have > -- > 2.34.1 > > The `test_path_exsists()`, `test_path_is_file()` and `test_path_is_dir()` helper functions are expected to be used within `test_expect_success` (or `test_expect_failure`) blocks [1]. However, in several of the above scripts, this condition is not met, making them unsuitable candidates for such modifications. Additionally, it is a good practice to make separate commits for logically separate changes and send multiple patches in a series [2-3], i.e submit one patch per test script. Thanks, Mahendra [References] 1. https://lore.kernel.org/git/CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com/ 2. https://git-scm.com/docs/MyFirstContribution 3. https://git-scm.com/docs/SubmittingPatches
diff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect index 93fb1a6578..49efa1301d 100644 --- a/t/chainlint/chained-subshell.expect +++ b/t/chainlint/chained-subshell.expect @@ -5,6 +5,6 @@ 6 ) && 7 8 cut "-d " -f actual | (read s1 s2 s3 && -9 test -f $s1 ?!LINT: missing '&&'?! +9 test_path_is_file "$s1" || error "$s1 should exist" 10 test $(cat $s2) = tree2path1 && 11 test $(cat $s3) = tree3path1) diff --git a/t/chainlint/chained-subshell.test b/t/chainlint/chained-subshell.test index 1f11f65398..e167335f1e 100644 --- a/t/chainlint/chained-subshell.test +++ b/t/chainlint/chained-subshell.test @@ -1,15 +1,15 @@ -test_expect_success 'chained-subshell' ' -# LINT: start of subshell chained to preceding command -mkdir sub && ( - cd sub && - foo the bar - nuff said -) && - -# LINT: preceding command pipes to subshell on same line -cut "-d " -f actual | (read s1 s2 s3 && -test -f $s1 -test $(cat $s2) = tree2path1 && -# LINT: closing subshell ")" correctly detected on same line as "$(...)" -test $(cat $s3) = tree3path1) -' +test_expect_success 'chained-subshell' ' +# LINT: start of subshell chained to preceding command +mkdir sub && ( + cd sub && + foo the bar + nuff said +) && + +# LINT: preceding command pipes to subshell on same line +cut "-d " -f actual | (read s1 s2 s3 && +test_path_is_file $s1 +test $(cat $s2) = tree2path1 && +# LINT: closing subshell ")" correctly detected on same line as "$(...)" +test $(cat $s3) = tree3path1) +' diff --git a/t/chainlint/function.expect b/t/chainlint/function.expect index 9e46a3554a..a0a465e6d4 100644 --- a/t/chainlint/function.expect +++ b/t/chainlint/function.expect @@ -4,7 +4,7 @@ 5 6 remove_object() { 7 file=$(sha1_file "$*") && -8 test -e "$file" ?!LINT: missing '&&'?! +8 test_path_exists "$file" || error "$file should exist" 9 rm -f "$file" 10 } ?!LINT: missing '&&'?! 11 diff --git a/t/chainlint/function.test b/t/chainlint/function.test index 763fcf3f87..841c60720c 100644 --- a/t/chainlint/function.test +++ b/t/chainlint/function.test @@ -1,15 +1,15 @@ -test_expect_success 'function' ' -# LINT: "()" in function definition not mistaken for subshell -sha1_file() { - echo "$*" | sed "s#..#.git/objects/&/#" -} && - -# LINT: broken &&-chain in function and after function -remove_object() { - file=$(sha1_file "$*") && - test -e "$file" - rm -f "$file" -} - -sha1_file arg && remove_object arg -' +test_expect_success 'function' ' +# LINT: "()" in function definition not mistaken for subshell +sha1_file() { + echo "$*" | sed "s#..#.git/objects/&/#" +} && + +# LINT: broken &&-chain in function and after function +remove_object() { + file=$(sha1_file "$*") && + test_path_exists "$file" || error "$file should exist" + rm -f "$file" +} + +sha1_file arg && remove_object arg +' diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh index 1b5864d2a7..b2055d4252 100644 --- a/t/interop/interop-lib.sh +++ b/t/interop/interop-lib.sh @@ -21,7 +21,7 @@ build_version () { sha1=$(git rev-parse "$1^{tree}") || return 1 dir=$BUILD_ROOT/$sha1 - if test -e "$dir/.built" + if test_path_exists "$dir/.built" then echo "$dir" return 0 @@ -37,7 +37,7 @@ build_version () { for config in config.mak config.mak.autogen config.status do - if test -e "$INTEROP_ROOT/../../$config" + if test_path_exists "$INTEROP_ROOT/../../$config" then cp "$INTEROP_ROOT/../../$config" "$dir/" || return 1 fi diff --git a/t/lib-httpd/apply-one-time-perl.sh b/t/lib-httpd/apply-one-time-perl.sh index d7f9fed6ae..83ede36efb 100644 --- a/t/lib-httpd/apply-one-time-perl.sh +++ b/t/lib-httpd/apply-one-time-perl.sh @@ -7,7 +7,7 @@ # # This can be used to simulate the effects of the repository changing in # between HTTP request-response pairs. -if test -f one-time-perl +if test_path_is_file one-time-perl then LC_ALL=C export LC_ALL diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh index d408d2caad..c39b816c45 100644 --- a/t/lib-httpd/nph-custom-auth.sh +++ b/t/lib-httpd/nph-custom-auth.sh @@ -41,7 +41,7 @@ then fi echo 'HTTP/1.1 401 Authorization Required' -if test -f "$CHALLENGE_FILE" +if test_path_is_file "$CHALLENGE_FILE" then sed -ne 's/^id=default.*response=//p' "$CHALLENGE_FILE" fi diff --git a/t/perf/p5302-pack-index.sh b/t/perf/p5302-pack-index.sh index 14c601bbf8..d3a3ea360f 100755 --- a/t/perf/p5302-pack-index.sh +++ b/t/perf/p5302-pack-index.sh @@ -9,7 +9,7 @@ test_perf_large_repo test_expect_success 'repack' ' git repack -ad && PACK=$(ls .git/objects/pack/*.pack | head -n1) && - test -f "$PACK" && + test_path_is_file "$PACK" && export PACK ' diff --git a/t/perf/p7527-builtin-fsmonitor.sh b/t/perf/p7527-builtin-fsmonitor.sh index 90164327e8..16e56cc197 100755 --- a/t/perf/p7527-builtin-fsmonitor.sh +++ b/t/perf/p7527-builtin-fsmonitor.sh @@ -46,7 +46,7 @@ export TMP_BR REPO=../repos/gen-many-files-"$PARAMS".git export REPO -if ! test -d $REPO +if ! test_path_is_dir $REPO then (cd ../repos; ./many-files.sh -d $PARAM_D -w $PARAM_W -f $PARAM_F) fi diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh index 8ab6d9c469..13eb33ae6f 100644 --- a/t/perf/perf-lib.sh +++ b/t/perf/perf-lib.sh @@ -117,7 +117,7 @@ test_perf_create_repo_from () { "$MODERN_GIT" init -q && test_perf_do_repo_symlink_config_ && mv .git/hooks .git/hooks-disabled 2>/dev/null && - if test -f .git/index.lock + if test_path_is_file .git/index.lock then # We may be copying a repo that can't run "git # status" due to a locked index. Since we have
This patch improves the Git test suite by converting old-style path checks to use modern Git test helpers. Specifically, we have replaced shell commands like `test -f` and `test -d` with the appropriate Git test helpers, such as `test_path_is_file` and `test_path_is_dir`. This enhances the readability and consistency of the test suite. The following tests were updated: - t/chainlint/cuddled-loop.test - t/chainlint/cuddled.test - t/chainlint/double-here-doc.test - t/chainlint/dqstring-line-splice.test - t/chainlint/dqstring-no-interpolate.test - t/chainlint/empty-here-doc.test - t/chainlint/exclamation.test - t/chainlint/exit-loop.test - t/chainlint/exit-subshell.test - t/chainlint/for-loop-abbreviated.test - t/chainlint/for-loop.test - t/chainlint/function.expect - t/chainlint/function.test - t/chainlint/here-doc-body-indent.test The changes have been verified by running the test suite to ensure no breaks or regressions. Command used to find instances: git grep 'test -[efd]' t/ Signed-off-by: Prachit Ingle <ingleprachit101@gmail.com> --- t/chainlint/chained-subshell.expect | 2 +- t/chainlint/chained-subshell.test | 30 ++++++++++++++--------------- t/chainlint/function.expect | 2 +- t/chainlint/function.test | 30 ++++++++++++++--------------- t/interop/interop-lib.sh | 4 ++-- t/lib-httpd/apply-one-time-perl.sh | 2 +- t/lib-httpd/nph-custom-auth.sh | 2 +- t/perf/p5302-pack-index.sh | 2 +- t/perf/p7527-builtin-fsmonitor.sh | 2 +- t/perf/perf-lib.sh | 2 +- 10 files changed, 39 insertions(+), 39 deletions(-)