| Message ID | 587e53053f02ad0867dc903688c8887602950bd3.1578372682.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: replace incorrect test_must_fail usage (part 2) | expand |
On Mon, Jan 6, 2020 at 11:53 PM Denton Liu <liu.denton@gmail.com> wrote: > t2018: use test_expect_code for failing git commands > > When we are expecting `git diff` to fail, we negate its status with > `!`. However, if git ever exits unexpectedly, say due to a segfault, we > would not be able to tell the difference between that and a controlled > failure. Use `test_expect_code 1 git diff` instead so that if an > unexpected failure occurs, we can catch it. > > We have some instances of `test_must_fail test_dirty_{un,}mergable`. > However, `test_must_fail` should only be used on git commands. Convert > these instances to use the `test_dirty_{un,}mergeable_discards_changes` > helper functions so that we remove the double-negation. I had to read all of this several times to understand what it was trying to say. I think what made it difficult was a combination of talking about using test_expect_code() for "failing git commands", coupled with the use "we" in "When we are ... we negate..." which (to me) sounds as if you are describing the _desired_ way of coding this, not the incorrect way. A possible rewrite: t2018: be more discerning when checking for expected exit codes Functions test_dirty_unmergeable() and test_dirty_mergeable() expect git-diff to exit with the specific code 1. However, rather than checking for that value explicitly, they instead negate the exit code. Unfortunately, this negation makes it impossible to distinguish the expected code from some other unexpected non-zero code, for instance, from a segmentation fault. Therefore, be more discerning by checking the exit code explicitly using test_expect_code(). Furthermore, some callers of those functions want to negate the result again, and do so with test_must_fail(). However, test_must_fail() should only be used with git commands. Address this by introducing a couple new tiny helper functions which test the exact condition expected (without the unnecessarily confusing double-negation). > While we're at it, remove redirections to /dev/null since output is > silenced when running without `-v` anyway. > Signed-off-by: Denton Liu <liu.denton@gmail.com>
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh index 61206a90fb..7ca55efc6b 100755 --- a/t/t2018-checkout-branch.sh +++ b/t/t2018-checkout-branch.sh @@ -34,7 +34,11 @@ do_checkout () { } test_dirty_unmergeable () { - ! git diff --exit-code >/dev/null + test_expect_code 1 git diff --exit-code +} + +test_dirty_unmergeable_discards_changes () { + git diff --exit-code } setup_dirty_unmergeable () { @@ -42,7 +46,11 @@ setup_dirty_unmergeable () { } test_dirty_mergeable () { - ! git diff --cached --exit-code >/dev/null + test_expect_code 1 git diff --cached --exit-code +} + +test_dirty_mergeable_discards_changes () { + git diff --cached --exit-code } setup_dirty_mergeable () { @@ -94,7 +102,7 @@ test_expect_success 'checkout -f -b to a new branch with unmergeable changes dis # still dirty and on branch1 do_checkout branch2 $HEAD1 "-f -b" && - test_must_fail test_dirty_unmergeable + test_dirty_unmergeable_discards_changes ' test_expect_success 'checkout -b to a new branch preserves mergeable changes' ' @@ -112,7 +120,7 @@ test_expect_success 'checkout -f -b to a new branch with mergeable changes disca test_when_finished git reset --hard HEAD && setup_dirty_mergeable && do_checkout branch2 $HEAD1 "-f -b" && - test_must_fail test_dirty_mergeable + test_dirty_mergeable_discards_changes ' test_expect_success 'checkout -b to an existing branch fails' ' @@ -163,7 +171,7 @@ test_expect_success 'checkout -B to an existing branch with unmergeable changes test_expect_success 'checkout -f -B to an existing branch with unmergeable changes discards changes' ' # still dirty and on branch1 do_checkout branch2 $HEAD1 "-f -B" && - test_must_fail test_dirty_unmergeable + test_dirty_unmergeable_discards_changes ' test_expect_success 'checkout -B to an existing branch preserves mergeable changes' ' @@ -180,7 +188,7 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes setup_dirty_mergeable && do_checkout branch2 $HEAD1 "-f -B" && - test_must_fail test_dirty_mergeable + test_dirty_mergeable_discards_changes ' test_expect_success 'checkout -b <describe>' '
When we are expecting `git diff` to fail, we negate its status with `!`. However, if git ever exits unexpectedly, say due to a segfault, we would not be able to tell the difference between that and a controlled failure. Use `test_expect_code 1 git diff` instead so that if an unexpected failure occurs, we can catch it. We have some instances of `test_must_fail test_dirty_{un,}mergable`. However, `test_must_fail` should only be used on git commands. Convert these instances to use the `test_dirty_{un,}mergeable_discards_changes` helper functions so that we remove the double-negation. While we're at it, remove redirections to /dev/null since output is silenced when running without `-v` anyway. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t2018-checkout-branch.sh | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)