| Message ID | 20231016152113.135970-1-anonolitunya@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5abb758118ebbc85fce114176722670c858f4b62 |
| Headers | show |
| Series | t/t7601: Modernize test scripts using functions | expand |
Dorcas AnonoLitunya <anonolitunya@gmail.com> writes: > Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions Let's try if we can pack a bit more information. For example Subject: [PATCH] t7601: use "test_path_is_file" etc. instead of "test -f" would clarify what kind of modernization is done by this patch. > The test script is currently using the command format 'test -f' to > check for existence or absence of files. "is currently using" -> "uses". > Replace it with new helper functions following the format > 'test_path_is_file'. I am not sure what role "the format" plays in this picture. test_path_is_file is not new---it has been around for quite a while. > Consequently, the patch also replaces the inverse command '! test -f' or > 'test ! -f' with new helper function following the format > 'test_path_is_missing' A bit more on this later. > This adjustment using helper functions makes the code more readable and > easier to understand. Looking good. If I were writing this, I'll make the whole thing more like this, though: t7601: use "test_path_is_file" etc. instead of "test -f" Some tests in t7601 use "test -f" and "test ! -f" to see if a path exists or is missing. Use test_path_is_file and test_path_is_missing helper functions to clarify these tests a bit better. This especially matters for the "missing" case, because "test ! -f F" will be happy if "F" exists as a directory, but the intent of the test is that "F" should not exist, even as a directory. > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh > index bd238d89b0..e08767df66 100755 > --- a/t/t7601-merge-pull-config.sh > +++ b/t/t7601-merge-pull-config.sh > @@ -349,13 +349,13 @@ test_expect_success 'Cannot rebase with multiple heads' ' > > test_expect_success 'merge c1 with c2' ' > git reset --hard c1 && > - test -f c0.c && > - test -f c1.c && > - test ! -f c2.c && > - test ! -f c3.c && > + test_path_is_file c0.c && > + test_path_is_file c1.c && > + test_path_is_missing c2.c && > + test_path_is_missing c3.c && The original says "We are happy if c2.c is not a file", so it would have been happy if by some mistake "git reset" created a directory there. But the _intent_ of the test is that we do not have anything at c2.c, and the updated code expresses it better.
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index bd238d89b0..e08767df66 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -349,13 +349,13 @@ test_expect_success 'Cannot rebase with multiple heads' ' test_expect_success 'merge c1 with c2' ' git reset --hard c1 && - test -f c0.c && - test -f c1.c && - test ! -f c2.c && - test ! -f c3.c && + test_path_is_file c0.c && + test_path_is_file c1.c && + test_path_is_missing c2.c && + test_path_is_missing c3.c && git merge c2 && - test -f c1.c && - test -f c2.c + test_path_is_file c1.c && + test_path_is_file c2.c ' test_expect_success 'fast-forward pull succeeds with "true" in pull.ff' ' @@ -411,8 +411,8 @@ test_expect_success 'merge c1 with c2 (ours in pull.twohead)' ' git reset --hard c1 && git config pull.twohead ours && git merge c2 && - test -f c1.c && - ! test -f c2.c + test_path_is_file c1.c && + test_path_is_missing c2.c ' test_expect_success 'merge c1 with c2 and c3 (recursive in pull.octopus)' ' @@ -431,10 +431,10 @@ test_expect_success 'merge c1 with c2 and c3 (recursive and octopus in pull.octo test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" && test "$(git rev-parse c3)" = "$(git rev-parse HEAD^3)" && git diff --exit-code && - test -f c0.c && - test -f c1.c && - test -f c2.c && - test -f c3.c + test_path_is_file c0.c && + test_path_is_file c1.c && + test_path_is_file c2.c && + test_path_is_file c3.c ' conflict_count()
The test script is currently using the command format 'test -f' to check for existence or absence of files. Replace it with new helper functions following the format 'test_path_is_file'. Consequently, the patch also replaces the inverse command '! test -f' or 'test ! -f' with new helper function following the format 'test_path_is_missing' This adjustment using helper functions makes the code more readable and easier to understand. Signed-off-by: Dorcas AnonoLitunya <anonolitunya@gmail.com> --- t/t7601-merge-pull-config.sh | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)