| Message ID | 20250101225915.65185-1-matteobagnolini2003@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 866ea877036ce581e0d3c130f527631cd8b36bdf |
| Headers | show |
| Series | t7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing. | expand |
On Wed, Jan 01, 2025 at 11:59:14PM +0100, matteobagnolini wrote:
The commit subject is overly long. We typically try to restrict them to
at most 72 characters. Furthermore, the part after the subsystem should
start with a lower-case letter. Suggestion:
t7110: replace `test -f` with `test_path_is_*` helpers
> `test -f` and `! test -f` do not provide clear error messages when they fail.
> To enanche debuggability, use `test_path_is_file` and `test_path_is_missing`,
s/enanche/enhance
> which instead provides more informative error messages.
s/provides/provide
It looks like you intended to start a new paragraph here. These should
typically be separated by an empty line.
> Note that `! test -f` checks if a path is not a file, while
> `test_path_is_missing` verifies that a path does not exist. In this specific
> case the tests are meant to check the absence of the path, making
> `test_path_is_missing` a valid replacement.
Makes sense.
> Signed-off-by: matteobagnolini <matteobagnolini2003@gmail.com>
We typically prefer proper legal names in the SOB.
> diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
> index 61669a2d21..9a335071af 100755
> --- a/t/t7110-reset-merge.sh
> +++ b/t/t7110-reset-merge.sh
> @@ -270,13 +270,13 @@ test_expect_success '--merge is ok with added/deleted merge' '
> git reset --hard third &&
> rm -f file2 &&
> test_must_fail git merge branch3 &&
> - ! test -f file2 &&
> - test -f file3 &&
> + test_path_is_missing file2 &&
> + test_path_is_file file3 &&
> git diff --exit-code file3 &&
> git diff --exit-code branch3 file3 &&
> git reset --merge HEAD &&
> - ! test -f file3 &&
> - ! test -f file2 &&
> + test_path_is_missing file3 &&
> + test_path_is_missing file2 &&
> git diff --exit-code --cached
> '
>
> @@ -284,8 +284,8 @@ test_expect_success '--keep fails with added/deleted merge' '
> git reset --hard third &&
> rm -f file2 &&
> test_must_fail git merge branch3 &&
> - ! test -f file2 &&
> - test -f file3 &&
> + test_path_is_missing file2 &&
> + test_path_is_file file3 &&
> git diff --exit-code file3 &&
> git diff --exit-code branch3 file3 &&
> test_must_fail git reset --keep HEAD 2>err.log &&
The changes themselves look obviously good to me, thanks!
Patrick
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh index 61669a2d21..9a335071af 100755 --- a/t/t7110-reset-merge.sh +++ b/t/t7110-reset-merge.sh @@ -270,13 +270,13 @@ test_expect_success '--merge is ok with added/deleted merge' ' git reset --hard third && rm -f file2 && test_must_fail git merge branch3 && - ! test -f file2 && - test -f file3 && + test_path_is_missing file2 && + test_path_is_file file3 && git diff --exit-code file3 && git diff --exit-code branch3 file3 && git reset --merge HEAD && - ! test -f file3 && - ! test -f file2 && + test_path_is_missing file3 && + test_path_is_missing file2 && git diff --exit-code --cached ' @@ -284,8 +284,8 @@ test_expect_success '--keep fails with added/deleted merge' ' git reset --hard third && rm -f file2 && test_must_fail git merge branch3 && - ! test -f file2 && - test -f file3 && + test_path_is_missing file2 && + test_path_is_file file3 && git diff --exit-code file3 && git diff --exit-code branch3 file3 && test_must_fail git reset --keep HEAD 2>err.log &&
`test -f` and `! test -f` do not provide clear error messages when they fail. To enanche debuggability, use `test_path_is_file` and `test_path_is_missing`, which instead provides more informative error messages. Note that `! test -f` checks if a path is not a file, while `test_path_is_missing` verifies that a path does not exist. In this specific case the tests are meant to check the absence of the path, making `test_path_is_missing` a valid replacement. Signed-off-by: matteobagnolini <matteobagnolini2003@gmail.com> --- t/t7110-reset-merge.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)