| Message ID | 48598e3f9859dc525ec878cd7f3eaadee8bb61b1.1590019226.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: replace incorrect test_must_fail usage (part 5) | expand |
On Wed, May 20, 2020 at 08:24:18PM -0400, Denton Liu wrote: > We are using `test_must_fail $command`. However, $command is not > necessarily a git command; it could be a test helper function. > > In an effort to stop using test_must_fail with non-git commands, instead > of invoking `test_must_fail $command`, run > `OVERWRITING_FAIL=test_must_fail $command` instead in > test_submodule_switch_common(). I think there are really two separate issues being addressed by this patch. One is using "test_must_fail" with a helper function. IMHO this is not something we should go too far in trying to deal with. It's mostly a style issue about how test_must_fail is meant to be used, and there's no real downside if we occasionally use it for a non-git command. I don't mind cleaning up spots where it's obviously misused, but in this case we're introducing a lot of new complexity to handle the "sometimes" nature of this call. And I'm not sure it's worth it on its own. > This is useful because currently, when we run a test helper function, we > just mark the whole thing as `test_must_fail`. However, it's possible > that the helper function might fail earlier or later than expected due > to an introduced bug. If this happens, then the test case will still > report as passing but it should really be marked as failing since it > didn't actually display the intended behaviour. Now this second concern I think is much more interesting, because it impacts the test results. And it's really not even about test_must_fail in particular, but is a general problem with checking failure in any compound operation. We may expect the early parts to succeed, but the later parts to fail, and we want to tell the difference. And that's true whether you're using "!", test_must_fail, etc. You solve it here by passing OVERWRITING_FAIL down into the callback functions. And that does work. But I think it may be easier to understand if we invert the responsibility. Let the outer caller specify two callbacks: one for setup/prep that must succeed, and one for a single operation where we might expect success or failure. The changes in lib-submodule-update look something like: @@ -326,7 +327,8 @@ test_submodule_switch_common() { ( cd submodule_update && git branch -t add_sub1 origin/add_sub1 && - $command add_sub1 && + $prep add_sub1 && + $command && test_superproject_content origin/add_sub1 && test_dir_is_empty sub1 && git submodule update --init --recursive && And in the caller we can simplify greatly: diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh index f916729a12..e3ae7c89f1 100755 --- a/t/t5572-pull-submodule.sh +++ b/t/t5572-pull-submodule.sh @@ -11,36 +11,13 @@ reset_branch_to_HEAD () { git branch --set-upstream-to="origin/$1" "$1" } -git_pull () { - reset_branch_to_HEAD "$1" && - git pull -} - # pulls without conflicts -test_submodule_switch "git_pull" - -git_pull_ff () { - reset_branch_to_HEAD "$1" && - git pull --ff -} - -test_submodule_switch "git_pull_ff" - -git_pull_ff_only () { - reset_branch_to_HEAD "$1" && - git pull --ff-only -} - -test_submodule_switch "git_pull_ff_only" - -git_pull_noff () { - reset_branch_to_HEAD "$1" && - git pull --no-ff -} - +test_submodule_switch "reset_branch_to_HEAD" "git pull" +test_submodule_switch "reset_branch_to_HEAD" "git pull -ff" +test_submodule_switch "reset_branch_to_HEAD" "git pull --ff-only" KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1 -test_submodule_switch "git_pull_noff" +test_submodule_switch "reset_branch_to_HEAD" "git pull --no-ff" test_expect_success 'pull --recurse-submodule setup' ' test_create_repo child && I suspect this approach involves touching more lines than yours (it has to add $prep everywhere $command is used). But IMHO the result is much simpler to understand, because there's no spooky-action-at-a-distance from magic environment variables. A more complete patch is below, which is enough to get t5572 passing. I think it would need the other test_submodule_switch() function updated, and other scripts would need to adapt to the 2-arg style. -Peff
On Wed, May 20, 2020 at 08:24:18PM -0400, Denton Liu wrote: > test_submodule_switch () { > - test_submodule_switch_func "git $1" > + test_submodule_switch_func "eval \$OVERWRITING_FAIL git $1" > } > [...] > test_submodule_forced_switch () { > command="$1" > KNOWN_FAILURE_FORCED_SWITCH_TESTS=1 > - test_submodule_switch_common "git $command" > + test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command" These both subject $1 to an extra layer of shell eval. That may not matter since we seem to only pass basic stuff. It could probably be avoided with an extra variable (though since we're passing along the eval to be eval'd, I think it gets tricky), so it may not be worth addressing. Though IMHO it is nicer still if we can avoid the eval altogether by doing something like the prep/command split I suggested in a sibling reply. -Peff
Hi Peff, On Thu, May 21, 2020 at 02:29:28PM -0400, Jeff King wrote: > > This is useful because currently, when we run a test helper function, we > > just mark the whole thing as `test_must_fail`. However, it's possible > > that the helper function might fail earlier or later than expected due > > to an introduced bug. If this happens, then the test case will still > > report as passing but it should really be marked as failing since it > > didn't actually display the intended behaviour. > > Now this second concern I think is much more interesting, because it > impacts the test results. And it's really not even about test_must_fail > in particular, but is a general problem with checking failure in any > compound operation. We may expect the early parts to succeed, but the > later parts to fail, and we want to tell the difference. And that's true > whether you're using "!", test_must_fail, etc. > > You solve it here by passing OVERWRITING_FAIL down into the callback > functions. And that does work. But I think it may be easier to > understand if we invert the responsibility. Let the outer caller specify > two callbacks: one for setup/prep that must succeed, and one for a > single operation where we might expect success or failure. I believe that we'll need a third optional argument. For example, in t3906, we have the following diff diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh index 6a7e801ca0..6e8dac9669 100755 --- a/t/t3906-stash-submodule.sh +++ b/t/t3906-stash-submodule.sh @@ -8,7 +8,11 @@ test_description='stash can handle submodules' git_stash () { git status -su >expect && ls -1pR * >>expect && - git read-tree -u -m "$1" && + $OVERWRITING_FAIL git read-tree -u -m "$1" && + if test -n "$OVERWRITING_FAIL" + then + return + fi && git stash && git status -su >actual && ls -1pR * >>actual && which means, when we're doing test_must_fail, we have to skip the remainder of the function otherwise, if the first command succeeds but it fails later in the test, then we report success even though it didn't fail at the point where we were expecting. I think that we'll have to have an optional third arg for $cleanup or something. The only thing that I'm worried about is that having three separate functions being passed in may be too messy. Aside from that, I like this approach much more than mine. Thanks, Denton
Jeff King <peff@peff.net> writes: > You solve it here by passing OVERWRITING_FAIL down into the callback > functions. And that does work. But I think it may be easier to > understand if we invert the responsibility. Let the outer caller specify > two callbacks: one for setup/prep that must succeed, and one for a > single operation where we might expect success or failure. > > The changes in lib-submodule-update look something like: > ... > And in the caller we can simplify greatly: > ... > +test_submodule_switch "reset_branch_to_HEAD" "git pull" > +test_submodule_switch "reset_branch_to_HEAD" "git pull -ff" > +test_submodule_switch "reset_branch_to_HEAD" "git pull --ff-only" > KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1 > KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1 > -test_submodule_switch "git_pull_noff" > +test_submodule_switch "reset_branch_to_HEAD" "git pull --no-ff" > > test_expect_success 'pull --recurse-submodule setup' ' > test_create_repo child && > > I suspect this approach involves touching more lines than yours (it has > to add $prep everywhere $command is used). But IMHO the result is much > simpler to understand, because there's no spooky-action-at-a-distance > from magic environment variables. Yes, spooky-action-at-a-distance was what made me go Eeeeek when I saw the original approach. > A more complete patch is below, which is enough to get t5572 passing. I > think it would need the other test_submodule_switch() function updated, > and other scripts would need to adapt to the 2-arg style. below where?
On Thu, May 21, 2020 at 02:11:53PM -0700, Junio C Hamano wrote: > > A more complete patch is below, which is enough to get t5572 passing. I > > think it would need the other test_submodule_switch() function updated, > > and other scripts would need to adapt to the 2-arg style. > > below where? Oops. --- t/lib-submodule-update.sh | 46 ++++++++++++++++++++++++++++++---------------- t/t5572-pull-submodule.sh | 31 ++++--------------------------- 2 files changed, 34 insertions(+), 43 deletions(-) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 64fc6487dd..34da02b7fa 100644 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -310,7 +310,8 @@ test_submodule_content () { # Internal function; use test_submodule_switch() or # test_submodule_forced_switch() instead. test_submodule_switch_common() { - command="$1" + prep="$1" + command="$2" ######################### Appearing submodule ######################### # Switching to a commit letting a submodule appear creates empty dir ... if test "$KNOWN_FAILURE_STASH_DOES_IGNORE_SUBMODULE_CHANGES" = 1 @@ -326,7 +327,8 @@ test_submodule_switch_common() { ( cd submodule_update && git branch -t add_sub1 origin/add_sub1 && - $command add_sub1 && + $prep add_sub1 && + $command && test_superproject_content origin/add_sub1 && test_dir_is_empty sub1 && git submodule update --init --recursive && @@ -341,7 +343,8 @@ test_submodule_switch_common() { cd submodule_update && mkdir sub1 && git branch -t add_sub1 origin/add_sub1 && - $command add_sub1 && + $prep add_sub1 && + $command && test_superproject_content origin/add_sub1 && test_dir_is_empty sub1 && git submodule update --init --recursive && @@ -356,7 +359,8 @@ test_submodule_switch_common() { ( cd submodule_update && git branch -t replace_file_with_sub1 origin/replace_file_with_sub1 && - $command replace_file_with_sub1 && + $prep replace_file_with_sub1 && + $command && test_superproject_content origin/replace_file_with_sub1 && test_dir_is_empty sub1 && git submodule update --init --recursive && @@ -380,7 +384,8 @@ test_submodule_switch_common() { ( cd submodule_update && git branch -t replace_directory_with_sub1 origin/replace_directory_with_sub1 && - $command replace_directory_with_sub1 && + $prep replace_directory_with_sub1 && + $command && test_superproject_content origin/replace_directory_with_sub1 && test_dir_is_empty sub1 && git submodule update --init --recursive && @@ -402,7 +407,8 @@ test_submodule_switch_common() { ( cd submodule_update && git branch -t remove_sub1 origin/remove_sub1 && - $command remove_sub1 && + $prep remove_sub1 && + $command && test_superproject_content origin/remove_sub1 && test_submodule_content sub1 origin/add_sub1 ) @@ -415,7 +421,8 @@ test_submodule_switch_common() { cd submodule_update && git branch -t remove_sub1 origin/remove_sub1 && replace_gitfile_with_git_dir sub1 && - $command remove_sub1 && + $prep remove_sub1 && + $command && test_superproject_content origin/remove_sub1 && test_git_directory_is_unchanged sub1 && test_submodule_content sub1 origin/add_sub1 @@ -443,7 +450,8 @@ test_submodule_switch_common() { ( cd submodule_update && git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory && - test_must_fail $command replace_sub1_with_directory && + $prep replace_sub1_with_directory && + test_must_fail $command && test_superproject_content origin/add_sub1 && test_submodule_content sub1 origin/add_sub1 ) @@ -456,7 +464,8 @@ test_submodule_switch_common() { cd submodule_update && git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory && replace_gitfile_with_git_dir sub1 && - test_must_fail $command replace_sub1_with_directory && + $prep replace_sub1_with_directory && + test_must_fail $command && test_superproject_content origin/add_sub1 && test_git_directory_is_unchanged sub1 && test_submodule_content sub1 origin/add_sub1 @@ -470,7 +479,8 @@ test_submodule_switch_common() { ( cd submodule_update && git branch -t replace_sub1_with_file origin/replace_sub1_with_file && - test_must_fail $command replace_sub1_with_file && + $prep replace_sub1_with_file && + test_must_fail $command && test_superproject_content origin/add_sub1 && test_submodule_content sub1 origin/add_sub1 ) @@ -484,7 +494,8 @@ test_submodule_switch_common() { cd submodule_update && git branch -t replace_sub1_with_file origin/replace_sub1_with_file && replace_gitfile_with_git_dir sub1 && - test_must_fail $command replace_sub1_with_file && + $prep replace_sub1_with_file && + test_must_fail $command && test_superproject_content origin/add_sub1 && test_git_directory_is_unchanged sub1 && test_submodule_content sub1 origin/add_sub1 @@ -508,7 +519,8 @@ test_submodule_switch_common() { ( cd submodule_update && git branch -t modify_sub1 origin/modify_sub1 && - $command modify_sub1 && + $prep modify_sub1 && + $command && test_superproject_content origin/modify_sub1 && test_submodule_content sub1 origin/add_sub1 && git submodule update && @@ -523,7 +535,8 @@ test_submodule_switch_common() { ( cd submodule_update && git branch -t invalid_sub1 origin/invalid_sub1 && - $command invalid_sub1 && + $prep invalid_sub1 && + $command && test_superproject_content origin/invalid_sub1 && test_submodule_content sub1 origin/add_sub1 && test_must_fail git submodule update && @@ -538,7 +551,8 @@ test_submodule_switch_common() { ( cd submodule_update && git branch -t valid_sub1 origin/valid_sub1 && - $command valid_sub1 && + $prep valid_sub1 && + $command && test_superproject_content origin/valid_sub1 && test_dir_is_empty sub1 && git submodule update --init --recursive && @@ -568,8 +582,8 @@ test_submodule_switch_common() { # } # test_submodule_switch "my_func" test_submodule_switch () { - command="$1" - test_submodule_switch_common "$command" + command="$2" + test_submodule_switch_common "$@" # An empty directory does not prevent the creation of a submodule of # the same name, but a file does. diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh index f916729a12..e3ae7c89f1 100755 --- a/t/t5572-pull-submodule.sh +++ b/t/t5572-pull-submodule.sh @@ -11,36 +11,13 @@ reset_branch_to_HEAD () { git branch --set-upstream-to="origin/$1" "$1" } -git_pull () { - reset_branch_to_HEAD "$1" && - git pull -} - # pulls without conflicts -test_submodule_switch "git_pull" - -git_pull_ff () { - reset_branch_to_HEAD "$1" && - git pull --ff -} - -test_submodule_switch "git_pull_ff" - -git_pull_ff_only () { - reset_branch_to_HEAD "$1" && - git pull --ff-only -} - -test_submodule_switch "git_pull_ff_only" - -git_pull_noff () { - reset_branch_to_HEAD "$1" && - git pull --no-ff -} - +test_submodule_switch "reset_branch_to_HEAD" "git pull" +test_submodule_switch "reset_branch_to_HEAD" "git pull -ff" +test_submodule_switch "reset_branch_to_HEAD" "git pull --ff-only" KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1 -test_submodule_switch "git_pull_noff" +test_submodule_switch "reset_branch_to_HEAD" "git pull --no-ff" test_expect_success 'pull --recurse-submodule setup' ' test_create_repo child &&
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index fb6c0f3d4f..a0274c03de 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -304,12 +304,15 @@ test_submodule_content () { # a removed submodule. # # Removing a submodule containing a .git directory must fail even when forced -# to protect the history! +# to protect the history! If we are testing this case, +# OVERWRITING_FAIL=test_must_fail, otherwise OVERWRITING_FAIL will be the empty +# string. # # Internal function; use test_submodule_switch_func(), test_submodule_switch_func(), # or test_submodule_forced_switch() instead. test_submodule_switch_common () { + OVERWRITING_FAIL= command="$1" ######################### Appearing submodule ######################### # Switching to a commit letting a submodule appear creates empty dir ... @@ -443,7 +446,9 @@ test_submodule_switch_common () { ( cd submodule_update && git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory && - test_must_fail $command replace_sub1_with_directory && + OVERWRITING_FAIL=test_must_fail && + $command replace_sub1_with_directory && + OVERWRITING_FAIL= && test_superproject_content origin/add_sub1 && test_submodule_content sub1 origin/add_sub1 ) @@ -456,7 +461,9 @@ test_submodule_switch_common () { cd submodule_update && git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory && replace_gitfile_with_git_dir sub1 && - test_must_fail $command replace_sub1_with_directory && + OVERWRITING_FAIL=test_must_fail && + $command replace_sub1_with_directory && + OVERWRITING_FAIL= && test_superproject_content origin/add_sub1 && test_git_directory_is_unchanged sub1 && test_submodule_content sub1 origin/add_sub1 @@ -470,7 +477,9 @@ test_submodule_switch_common () { ( cd submodule_update && git branch -t replace_sub1_with_file origin/replace_sub1_with_file && - test_must_fail $command replace_sub1_with_file && + OVERWRITING_FAIL=test_must_fail && + $command replace_sub1_with_file && + OVERWRITING_FAIL= && test_superproject_content origin/add_sub1 && test_submodule_content sub1 origin/add_sub1 ) @@ -484,7 +493,9 @@ test_submodule_switch_common () { cd submodule_update && git branch -t replace_sub1_with_file origin/replace_sub1_with_file && replace_gitfile_with_git_dir sub1 && - test_must_fail $command replace_sub1_with_file && + OVERWRITING_FAIL=test_must_fail && + $command replace_sub1_with_file && + OVERWRITING_FAIL= && test_superproject_content origin/add_sub1 && test_git_directory_is_unchanged sub1 && test_submodule_content sub1 origin/add_sub1 @@ -559,6 +570,11 @@ test_submodule_switch_common () { # conditions, set the appropriate KNOWN_FAILURE_* variable used in the tests # below to 1. # +# Removing a submodule containing a .git directory must fail even when forced +# to protect the history! If we are testing this case, +# OVERWRITING_FAIL=test_must_fail, otherwise OVERWRITING_FAIL will be the empty +# string. +# # Use as follows: # # my_func () { @@ -568,6 +584,7 @@ test_submodule_switch_common () { # } # test_submodule_switch_func "my_func" test_submodule_switch_func () { + OVERWRITING_FAIL= command="$1" test_submodule_switch_common "$command" @@ -580,7 +597,7 @@ test_submodule_switch_func () { cd submodule_update && git branch -t add_sub1 origin/add_sub1 && >sub1 && - test_must_fail $command add_sub1 && + OVERWRITING_FAIL=test_must_fail $command add_sub1 && test_superproject_content origin/no_submodule && test_must_be_empty sub1 ) @@ -588,7 +605,7 @@ test_submodule_switch_func () { } test_submodule_switch () { - test_submodule_switch_func "git $1" + test_submodule_switch_func "eval \$OVERWRITING_FAIL git $1" } # Same as test_submodule_switch(), except that throwing away local changes in @@ -596,7 +613,7 @@ test_submodule_switch () { test_submodule_forced_switch () { command="$1" KNOWN_FAILURE_FORCED_SWITCH_TESTS=1 - test_submodule_switch_common "git $command" + test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command" # When forced, a file in the superproject does not prevent creating a # submodule of the same name. diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh index 788605ccc0..c6a7f584ed 100755 --- a/t/t3426-rebase-submodule.sh +++ b/t/t3426-rebase-submodule.sh @@ -17,7 +17,7 @@ git_rebase () { git status -su >actual && ls -1pR * >>actual && test_cmp expect actual && - git rebase "$1" + $OVERWRITING_FAIL git rebase "$1" } test_submodule_switch_func "git_rebase" @@ -35,7 +35,7 @@ git_rebase_interactive () { test_cmp expect actual && set_fake_editor && echo "fake-editor.sh" >.git/info/exclude && - git rebase -i "$1" + $OVERWRITING_FAIL git rebase -i "$1" } test_submodule_switch_func "git_rebase_interactive" diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh index 95a7f64471..e41f0f2009 100755 --- a/t/t3513-revert-submodule.sh +++ b/t/t3513-revert-submodule.sh @@ -15,6 +15,11 @@ git_revert () { git status -su >expect && ls -1pR * >>expect && tar cf "$TRASH_DIRECTORY/tmp.tar" * && + $OVERWRITING_FAIL git checkout "$1" && + if test -n "$OVERWRITING_FAIL" + then + return + fi && git checkout "$1" && git revert HEAD && rm -rf * && diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh index 6a7e801ca0..6e8dac9669 100755 --- a/t/t3906-stash-submodule.sh +++ b/t/t3906-stash-submodule.sh @@ -8,7 +8,11 @@ test_description='stash can handle submodules' git_stash () { git status -su >expect && ls -1pR * >>expect && - git read-tree -u -m "$1" && + $OVERWRITING_FAIL git read-tree -u -m "$1" && + if test -n "$OVERWRITING_FAIL" + then + return + fi && git stash && git status -su >actual && ls -1pR * >>actual && diff --git a/t/t4137-apply-submodule.sh b/t/t4137-apply-submodule.sh index b645e303a0..dd08d9e1a4 100755 --- a/t/t4137-apply-submodule.sh +++ b/t/t4137-apply-submodule.sh @@ -6,13 +6,15 @@ test_description='git apply handling submodules' . "$TEST_DIRECTORY"/lib-submodule-update.sh apply_index () { - git diff --ignore-submodules=dirty "..$1" | git apply --index - + git diff --ignore-submodules=dirty "..$1" >diff && + $OVERWRITING_FAIL git apply --index - <diff } test_submodule_switch_func "apply_index" apply_3way () { - git diff --ignore-submodules=dirty "..$1" | git apply --3way - + git diff --ignore-submodules=dirty "..$1" >diff && + $OVERWRITING_FAIL git apply --3way - <diff } test_submodule_switch_func "apply_3way" diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh index 1b179d5f45..b0fe78a956 100755 --- a/t/t4255-am-submodule.sh +++ b/t/t4255-am-submodule.sh @@ -6,13 +6,15 @@ test_description='git am handling submodules' . "$TEST_DIRECTORY"/lib-submodule-update.sh am () { - git format-patch --stdout --ignore-submodules=dirty "..$1" | git am - + git format-patch --stdout --ignore-submodules=dirty "..$1" >patch && + $OVERWRITING_FAIL git am - <patch } test_submodule_switch_func "am" am_3way () { - git format-patch --stdout --ignore-submodules=dirty "..$1" | git am --3way - + git format-patch --stdout --ignore-submodules=dirty "..$1" >patch && + $OVERWRITING_FAIL git am --3way - <patch } KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1 diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh index f911bf631e..4dd9913b6b 100755 --- a/t/t5572-pull-submodule.sh +++ b/t/t5572-pull-submodule.sh @@ -13,7 +13,7 @@ reset_branch_to_HEAD () { git_pull () { reset_branch_to_HEAD "$1" && - git pull + $OVERWRITING_FAIL git pull } # pulls without conflicts @@ -21,21 +21,21 @@ test_submodule_switch_func "git_pull" git_pull_ff () { reset_branch_to_HEAD "$1" && - git pull --ff + $OVERWRITING_FAIL git pull --ff } test_submodule_switch_func "git_pull_ff" git_pull_ff_only () { reset_branch_to_HEAD "$1" && - git pull --ff-only + $OVERWRITING_FAIL git pull --ff-only } test_submodule_switch_func "git_pull_ff_only" git_pull_noff () { reset_branch_to_HEAD "$1" && - git pull --no-ff + $OVERWRITING_FAIL git pull --no-ff } KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1 diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh index 0e0cdf638d..759890b721 100755 --- a/t/t6041-bisect-submodule.sh +++ b/t/t6041-bisect-submodule.sh @@ -10,7 +10,11 @@ git_bisect () { ls -1pR * >>expect && tar cf "$TRASH_DIRECTORY/tmp.tar" * && GOOD=$(git rev-parse --verify HEAD) && - git checkout "$1" && + $OVERWRITING_FAIL git checkout "$1" && + if test -n "$OVERWRITING_FAIL" + then + return + fi && echo "foo" >bar && git add bar && git commit -m "bisect bad" &&
We are using `test_must_fail $command`. However, $command is not necessarily a git command; it could be a test helper function. In an effort to stop using test_must_fail with non-git commands, instead of invoking `test_must_fail $command`, run `OVERWRITING_FAIL=test_must_fail $command` instead in test_submodule_switch_common(). In the case where $command is a git command, test_submodule_switch_common() is called by one of test_submodule_switch() or test_submodule_forced_switch(). In those two functions, pass $command like this: test_submodule_switch_common "eval \$OVERWRITING_FAIL git $command" When $command is a helper function, the parent function calling test_submodule_switch_common() is test_submodule_switch_func(). For all test_submodule_switch_func() invocations, increase the granularity of the argument test helper function by prefixing the git invocation which is meant to fail with $OVERWRITING_FAIL like this: $OVERWRITING_FAIL git checkout "$1" This is useful because currently, when we run a test helper function, we just mark the whole thing as `test_must_fail`. However, it's possible that the helper function might fail earlier or later than expected due to an introduced bug. If this happens, then the test case will still report as passing but it should really be marked as failing since it didn't actually display the intended behaviour. While we're at it, some helper functions have git commands piping into another git command. Break these pipes up into two separate invocations with a file buffer so that the return code of the first command is not lost. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/lib-submodule-update.sh | 33 +++++++++++++++++++++++++-------- t/t3426-rebase-submodule.sh | 4 ++-- t/t3513-revert-submodule.sh | 5 +++++ t/t3906-stash-submodule.sh | 6 +++++- t/t4137-apply-submodule.sh | 6 ++++-- t/t4255-am-submodule.sh | 6 ++++-- t/t5572-pull-submodule.sh | 8 ++++---- t/t6041-bisect-submodule.sh | 6 +++++- 8 files changed, 54 insertions(+), 20 deletions(-)