Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell
On Tue, Feb 27, 2018 at 10:17 PM, Junio C Hamano wrote: > SZEDER Gábor writes: > >> + git read-tree -i -m $c3 2>actual-err && >> + test_must_be_empty expected-err && >> + git update-index --ignore-missing --refresh 2>>actual-err && >> + test_must_be_empty expected-err && >> + git merge-recursive $c0 -- $c3 $c7 2>>actual-err && >> + test_must_be_empty expected-err && >> + git ls-files -s >actual-files 2>>actual-err && >> + test_must_be_empty expected-err > > Also, with the error output of individual steps tested like this > (assuming that test-must-be-empty checks are to be done on > the actual-err file, not ecpected-err that nobody creates), I do not > see a point in appending to the file. So perhaps squash this in? Agreed again. > t/t3030-merge-recursive.sh | 22 +++--- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh > index cbeea1cf94..3563e77b37 100755 > --- a/t/t3030-merge-recursive.sh > +++ b/t/t3030-merge-recursive.sh > @@ -526,13 +526,13 @@ test_expect_success 'merge-recursive w/ empty work tree > - ours has rename' ' > export GIT_INDEX_FILE && > mkdir "$GIT_WORK_TREE" && > git read-tree -i -m $c7 2>actual-err && > - test_must_be_empty expected-err && > + test_must_be_empty actual-err && > git update-index --ignore-missing --refresh 2>actual-err && > - test_must_be_empty expected-err && > + test_must_be_empty actual-err && > git merge-recursive $c0 -- $c7 $c3 2>actual-err && > - test_must_be_empty expected-err && > + test_must_be_empty actual-err && > git ls-files -s >actual-files 2>actual-err && > - test_must_be_empty expected-err > + test_must_be_empty actual-err > ) && > cat >expected-files <<-EOF && > 100644 $o3 0b/c > @@ -551,13 +551,13 @@ test_expect_success 'merge-recursive w/ empty work tree > - theirs has rename' ' > export GIT_INDEX_FILE && > mkdir "$GIT_WORK_TREE" && > git read-tree -i -m $c3 2>actual-err && > - test_must_be_empty expected-err && > - git update-index --ignore-missing --refresh 2>>actual-err && > - test_must_be_empty expected-err && > - git merge-recursive $c0 -- $c3 $c7 2>>actual-err && > - test_must_be_empty expected-err && > - git ls-files -s >actual-files 2>>actual-err && > - test_must_be_empty expected-err > + test_must_be_empty actual-err && > + git update-index --ignore-missing --refresh 2>actual-err && > + test_must_be_empty actual-err && > + git merge-recursive $c0 -- $c3 $c7 2>actual-err && > + test_must_be_empty actual-err && > + git ls-files -s >actual-files 2>actual-err && > + test_must_be_empty actual-err > ) && > cat >expected-files <<-EOF && > 100644 $o3 0b/c
Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell
On Tue, Feb 27, 2018 at 10:10 PM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> SZEDER Gábor writes:
>>
>>> The two test checking 'git mmerge-recursive' in an empty worktree in
>>> ...
>>> GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
>>> export GIT_INDEX_FILE &&
>>> mkdir "$GIT_WORK_TREE" &&
>>> -git read-tree -i -m $c7 &&
>>> -git update-index --ignore-missing --refresh &&
>>> -git merge-recursive $c0 -- $c7 $c3 &&
>>> -git ls-files -s >actual-files
>>> -) 2>actual-err &&
>>> ->expected-err &&
>>> +git read-tree -i -m $c7 2>actual-err &&
>>> +test_must_be_empty expected-err &&
>>> +git update-index --ignore-missing --refresh 2>actual-err &&
>>> +test_must_be_empty expected-err &&
>>> +git merge-recursive $c0 -- $c7 $c3 2>actual-err &&
>>> +test_must_be_empty expected-err &&
>>> +git ls-files -s >actual-files 2>actual-err &&
>>> +test_must_be_empty expected-err
>>
>> Where do the contents of all of these expected-err files come from?
>> Should all of the test_must_be_empty checks be checking actual-err
>> instead?
Ugh, I messed that up.
> And the reason why your pre-submission testing did not catch may be
> because test_must_be_empty is broken? I wonder if this is a good
> way forward to catch a possible bug like this.
Yeah. 'test -s file' means "exists and has a size greater than zero",
so the missing file doesn't trigger the error code path.
> Of course, if somebody was using the helepr for "must be either
> missing or empty", this change will break it, but I somehow doubt
> it.
FWIW, I just run the test suite with this change added, and there were
no failures. I think it's a good change.
> A program that creates/opens and writes an error message only
> when an error is detected is certainly possible, and could be tested
> with the current test_must_be_empty this way:
>
> rm -f actual-err &&
> git frotz --error-to=actual-err &&
> test_must_be_empty actual-err
>
> but then the last step in such a test like the above is more natural
> to check if actual-err _exists_ in the first place anyway, so...
>
> t/test-lib-functions.sh | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 37eb34044a..6cfbee60e4 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -772,7 +772,11 @@ verbose () {
> # otherwise.
>
> test_must_be_empty () {
> - if test -s "$1"
> + if ! test -f "$1"
> + then
> + echo "'$1' is missing"
> + return 1
> + elif test -s "$1"
> then
> echo "'$1' is not empty, it contains:"
> cat "$1"
Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell
SZEDER Gábor writes: > + git read-tree -i -m $c3 2>actual-err && > + test_must_be_empty expected-err && > + git update-index --ignore-missing --refresh 2>>actual-err && > + test_must_be_empty expected-err && > + git merge-recursive $c0 -- $c3 $c7 2>>actual-err && > + test_must_be_empty expected-err && > + git ls-files -s >actual-files 2>>actual-err && > + test_must_be_empty expected-err Also, with the error output of individual steps tested like this (assuming that test-must-be-empty checks are to be done on the actual-err file, not ecpected-err that nobody creates), I do not see a point in appending to the file. So perhaps squash this in? t/t3030-merge-recursive.sh | 22 +++--- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh index cbeea1cf94..3563e77b37 100755 --- a/t/t3030-merge-recursive.sh +++ b/t/t3030-merge-recursive.sh @@ -526,13 +526,13 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' ' export GIT_INDEX_FILE && mkdir "$GIT_WORK_TREE" && git read-tree -i -m $c7 2>actual-err && - test_must_be_empty expected-err && + test_must_be_empty actual-err && git update-index --ignore-missing --refresh 2>actual-err && - test_must_be_empty expected-err && + test_must_be_empty actual-err && git merge-recursive $c0 -- $c7 $c3 2>actual-err && - test_must_be_empty expected-err && + test_must_be_empty actual-err && git ls-files -s >actual-files 2>actual-err && - test_must_be_empty expected-err + test_must_be_empty actual-err ) && cat >expected-files <<-EOF && 100644 $o3 0b/c @@ -551,13 +551,13 @@ test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' ' export GIT_INDEX_FILE && mkdir "$GIT_WORK_TREE" && git read-tree -i -m $c3 2>actual-err && - test_must_be_empty expected-err && - git update-index --ignore-missing --refresh 2>>actual-err && - test_must_be_empty expected-err && - git merge-recursive $c0 -- $c3 $c7 2>>actual-err && - test_must_be_empty expected-err && - git ls-files -s >actual-files 2>>actual-err && - test_must_be_empty expected-err + test_must_be_empty actual-err && + git update-index --ignore-missing --refresh 2>actual-err && + test_must_be_empty actual-err && + git merge-recursive $c0 -- $c3 $c7 2>actual-err && + test_must_be_empty actual-err && + git ls-files -s >actual-files 2>actual-err && + test_must_be_empty actual-err ) && cat >expected-files <<-EOF && 100644 $o3 0b/c
Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell
Junio C Hamano writes:
> SZEDER Gábor writes:
>
>> The two test checking 'git mmerge-recursive' in an empty worktree in
>> ...
>> GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
>> export GIT_INDEX_FILE &&
>> mkdir "$GIT_WORK_TREE" &&
>> -git read-tree -i -m $c7 &&
>> -git update-index --ignore-missing --refresh &&
>> -git merge-recursive $c0 -- $c7 $c3 &&
>> -git ls-files -s >actual-files
>> -) 2>actual-err &&
>> ->expected-err &&
>> +git read-tree -i -m $c7 2>actual-err &&
>> +test_must_be_empty expected-err &&
>> +git update-index --ignore-missing --refresh 2>actual-err &&
>> +test_must_be_empty expected-err &&
>> +git merge-recursive $c0 -- $c7 $c3 2>actual-err &&
>> +test_must_be_empty expected-err &&
>> +git ls-files -s >actual-files 2>actual-err &&
>> +test_must_be_empty expected-err
>
> Where do the contents of all of these expected-err files come from?
> Should all of the test_must_be_empty checks be checking actual-err
> instead?
And the reason why your pre-submission testing did not catch may be
because test_must_be_empty is broken? I wonder if this is a good
way forward to catch a possible bug like this.
Of course, if somebody was using the helepr for "must be either
missing or empty", this change will break it, but I somehow doubt
it. A program that creates/opens and writes an error message only
when an error is detected is certainly possible, and could be tested
with the current test_must_be_empty this way:
rm -f actual-err &&
git frotz --error-to=actual-err &&
test_must_be_empty actual-err
but then the last step in such a test like the above is more natural
to check if actual-err _exists_ in the first place anyway, so...
t/test-lib-functions.sh | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 37eb34044a..6cfbee60e4 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -772,7 +772,11 @@ verbose () {
# otherwise.
test_must_be_empty () {
- if test -s "$1"
+ if ! test -f "$1"
+ then
+ echo "'$1' is missing"
+ return 1
+ elif test -s "$1"
then
echo "'$1' is not empty, it contains:"
cat "$1"
Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell
SZEDER Gábor writes: > The two test checking 'git mmerge-recursive' in an empty worktree in > ... > GIT_INDEX_FILE="$PWD/ours-has-rename-index" && > export GIT_INDEX_FILE && > mkdir "$GIT_WORK_TREE" && > - git read-tree -i -m $c7 && > - git update-index --ignore-missing --refresh && > - git merge-recursive $c0 -- $c7 $c3 && > - git ls-files -s >actual-files > - ) 2>actual-err && > - >expected-err && > + git read-tree -i -m $c7 2>actual-err && > + test_must_be_empty expected-err && > + git update-index --ignore-missing --refresh 2>actual-err && > + test_must_be_empty expected-err && > + git merge-recursive $c0 -- $c7 $c3 2>actual-err && > + test_must_be_empty expected-err && > + git ls-files -s >actual-files 2>actual-err && > + test_must_be_empty expected-err Where do the contents of all of these expected-err files come from? Should all of the test_must_be_empty checks be checking actual-err instead?
Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell
On Fri, Feb 23, 2018 at 6:39 PM, SZEDER Gábor wrote: > The two test checking 'git mmerge-recursive' in an empty worktree in s/mmerge/merge/, I guess. > 't3030-merge-recursive.sh' fail when the test script is run with '-x' > tracing (and using a shell other than a Bash version supporting > BASH_XTRACEFD). The reason for those failures is that the tests check > the emptiness of a subshell's stderr, which includes the trace of > commands executed in that subshell as well, throwing off the emptiness > check. > > Note that both subshells execute four git commands each, meaning that > checking the emptiness of the whole subshell implicitly ensures that > not only 'git merge-recursive' but none of the other three commands > outputs anything to their stderr. Note also that if one of those > commands were to output anything on its stderr, then the current > combined check would not tell us which one of those four commands the > unexpected output came from. > > Save the stderr of those four commands only instead of the whole > subshell, so it remains free from tracing output, and save and check > them individually, so they will show us from which command the > unexpected output came from. > > After this change t3030 passes with '-x', even when running with > /bin/sh. > > Signed-off-by: SZEDER Gábor
[PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell
The two test checking 'git mmerge-recursive' in an empty worktree in 't3030-merge-recursive.sh' fail when the test script is run with '-x' tracing (and using a shell other than a Bash version supporting BASH_XTRACEFD). The reason for those failures is that the tests check the emptiness of a subshell's stderr, which includes the trace of commands executed in that subshell as well, throwing off the emptiness check. Note that both subshells execute four git commands each, meaning that checking the emptiness of the whole subshell implicitly ensures that not only 'git merge-recursive' but none of the other three commands outputs anything to their stderr. Note also that if one of those commands were to output anything on its stderr, then the current combined check would not tell us which one of those four commands the unexpected output came from. Save the stderr of those four commands only instead of the whole subshell, so it remains free from tracing output, and save and check them individually, so they will show us from which command the unexpected output came from. After this change t3030 passes with '-x', even when running with /bin/sh. Signed-off-by: SZEDER Gábor --- t/t3030-merge-recursive.sh | 36 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh index cdc38fe5d1..cbeea1cf94 100755 --- a/t/t3030-merge-recursive.sh +++ b/t/t3030-merge-recursive.sh @@ -525,20 +525,22 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' ' GIT_INDEX_FILE="$PWD/ours-has-rename-index" && export GIT_INDEX_FILE && mkdir "$GIT_WORK_TREE" && - git read-tree -i -m $c7 && - git update-index --ignore-missing --refresh && - git merge-recursive $c0 -- $c7 $c3 && - git ls-files -s >actual-files - ) 2>actual-err && - >expected-err && + git read-tree -i -m $c7 2>actual-err && + test_must_be_empty expected-err && + git update-index --ignore-missing --refresh 2>actual-err && + test_must_be_empty expected-err && + git merge-recursive $c0 -- $c7 $c3 2>actual-err && + test_must_be_empty expected-err && + git ls-files -s >actual-files 2>actual-err && + test_must_be_empty expected-err + ) && cat >expected-files <<-EOF && 100644 $o3 0b/c 100644 $o0 0c 100644 $o0 0d/e 100644 $o0 0e EOF - test_cmp expected-files actual-files && - test_cmp expected-err actual-err + test_cmp expected-files actual-files ' test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' ' @@ -548,20 +550,22 @@ test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' ' GIT_INDEX_FILE="$PWD/theirs-has-rename-index" && export GIT_INDEX_FILE && mkdir "$GIT_WORK_TREE" && - git read-tree -i -m $c3 && - git update-index --ignore-missing --refresh && - git merge-recursive $c0 -- $c3 $c7 && - git ls-files -s >actual-files - ) 2>actual-err && - >expected-err && + git read-tree -i -m $c3 2>actual-err && + test_must_be_empty expected-err && + git update-index --ignore-missing --refresh 2>>actual-err && + test_must_be_empty expected-err && + git merge-recursive $c0 -- $c3 $c7 2>>actual-err && + test_must_be_empty expected-err && + git ls-files -s >actual-files 2>>actual-err && + test_must_be_empty expected-err + ) && cat >expected-files <<-EOF && 100644 $o3 0b/c 100644 $o0 0c 100644 $o0 0d/e 100644 $o0 0e EOF - test_cmp expected-files actual-files && - test_cmp expected-err actual-err + test_cmp expected-files actual-files ' test_expect_success 'merge removes empty directories' ' -- 2.16.2.400.g911b7cc0da
