[02/17] t0001 (mingw): do not expect a specific order of stdout/stderr

Commit Message

Linus Arver via GitGitGadget June 18, 2019, 12:23 p.m. UTC

From: Johannes Schindelin <[e-mail]>

When redirecting stdout/stderr to the same file, we cannot guarantee
that stdout will come first.

In fact, in this test case, it seems that an MSVC build always prints
stderr first.

In any case, this test case does not want to verify the *order* but
the *presence* of both outputs, so let's relax the test a little.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 t/t0001-init.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Eric Sunshine June 18, 2019, 11:12 p.m. UTC | #1

On Tue, Jun 18, 2019 at 8:24 AM Johannes Schindelin via GitGitGadget
<[e-mail]> wrote:
> When redirecting stdout/stderr to the same file, we cannot guarantee
> that stdout will come first.
>
> In fact, in this test case, it seems that an MSVC build always prints
> stderr first.
>
> In any case, this test case does not want to verify the *order* but
> the *presence* of both outputs, so let's relax the test a little.
>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> @@ -474,7 +474,8 @@ test_expect_success MINGW 'redirect std handles' '
>         printf ".git\nfatal: Needed a single revision\n" >expect &&
> -       test_cmp expect output.txt
> +       sort <output.txt >output.sorted &&
> +       test_cmp expect output.sorted

It was quite surprising to see this sorting only 'output' but not
'expect'. I see now that 'output' is already "sorted" (in that sense),
but it feels fragile. More robust would be to sort 'expect' as well:

    printf ".git\nfatal: Needed a single revision\n" | sort >expect &&

This would protect against the next person who modifies the 'printf'
testing on Unix and Windows/gcc and thinking all is well even though
the change might make the test fail for an MSVC build.

Johannes Sixt June 19, 2019, 6:19 a.m. UTC | #2

Am 19.06.19 um 01:12 schrieb Eric Sunshine:
> On Tue, Jun 18, 2019 at 8:24 AM Johannes Schindelin via GitGitGadget
> <[e-mail]> wrote:
>> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
>> @@ -474,7 +474,8 @@ test_expect_success MINGW 'redirect std handles' '
>>         printf ".git\nfatal: Needed a single revision\n" >expect &&
>> -       test_cmp expect output.txt
>> +       sort <output.txt >output.sorted &&
>> +       test_cmp expect output.sorted
> 
> It was quite surprising to see this sorting only 'output' but not
> 'expect'. I see now that 'output' is already "sorted" (in that sense),
> but it feels fragile. More robust would be to sort 'expect' as well:
> 
>     printf ".git\nfatal: Needed a single revision\n" | sort >expect &&

Following Dscho's recent objection elsewhere that tests tend to check
for much more than regressions, wouldn't it be logical to write these as

	grep -F .git" output.txt &&
	test_i18n_grep "Needed a single rev" output.txt

without an 'expect' file at all?

-- Hannes

Eric Sunshine June 19, 2019, 6:40 a.m. UTC | #3

On Wed, Jun 19, 2019 at 2:19 AM Johannes Sixt <[e-mail]> wrote:
> Am 19.06.19 um 01:12 schrieb Eric Sunshine:
> > On Tue, Jun 18, 2019 at 8:24 AM Johannes Schindelin via GitGitGadget
> > <[e-mail]> wrote:
> >>         printf ".git\nfatal: Needed a single revision\n" >expect &&
> >> -       test_cmp expect output.txt
> >> +       sort <output.txt >output.sorted &&
> >> +       test_cmp expect output.sorted
> >
> > It was quite surprising to see this sorting only 'output' but not
> > 'expect'. I see now that 'output' is already "sorted" (in that sense),
> > but it feels fragile. More robust would be to sort 'expect' as well:
> >
> >     printf ".git\nfatal: Needed a single revision\n" | sort >expect &&
>
> Following Dscho's recent objection elsewhere that tests tend to check
> for much more than regressions, wouldn't it be logical to write these as
>
>         grep -F .git" output.txt &&
>         test_i18n_grep "Needed a single rev" output.txt
>
> without an 'expect' file at all?

I considered suggesting that, as well, as being more obvious and less
fragile (with the exception that "Needed a single rev" isn't currently
localizable in builtin/rev-parse.c, so plain 'grep' instead of
'test_i18n_grep').

Johannes Schindelin June 19, 2019, 11:26 a.m. UTC | #4

Hi Eric & Hannes,

On Wed, 19 Jun 2019, Eric Sunshine wrote:

> On Wed, Jun 19, 2019 at 2:19 AM Johannes Sixt <[e-mail]> wrote:
> > Am 19.06.19 um 01:12 schrieb Eric Sunshine:
> > > On Tue, Jun 18, 2019 at 8:24 AM Johannes Schindelin via GitGitGadget
> > > <[e-mail]> wrote:
> > >>         printf ".git\nfatal: Needed a single revision\n" >expect &&
> > >> -       test_cmp expect output.txt
> > >> +       sort <output.txt >output.sorted &&
> > >> +       test_cmp expect output.sorted
> > >
> > > It was quite surprising to see this sorting only 'output' but not
> > > 'expect'. I see now that 'output' is already "sorted" (in that sense),
> > > but it feels fragile. More robust would be to sort 'expect' as well:
> > >
> > >     printf ".git\nfatal: Needed a single revision\n" | sort >expect &&
> >
> > Following Dscho's recent objection elsewhere that tests tend to check
> > for much more than regressions, wouldn't it be logical to write these as
> >
> >         grep -F .git" output.txt &&
> >         test_i18n_grep "Needed a single rev" output.txt
> >
> > without an 'expect' file at all?
>
> I considered suggesting that, as well, as being more obvious and less
> fragile (with the exception that "Needed a single rev" isn't currently
> localizable in builtin/rev-parse.c, so plain 'grep' instead of
> 'test_i18n_grep').

Valid points all around, thank you so much!

The next iteration will have the two `grep`s instead,
Dscho

Johannes Schindelin June 19, 2019, 11:30 a.m. UTC | #5

Hi Eric,

On Wed, 19 Jun 2019, Johannes Schindelin wrote:

> On Wed, 19 Jun 2019, Eric Sunshine wrote:
>
> > On Wed, Jun 19, 2019 at 2:19 AM Johannes Sixt <[e-mail]> wrote:
> > > Am 19.06.19 um 01:12 schrieb Eric Sunshine:
> > > > On Tue, Jun 18, 2019 at 8:24 AM Johannes Schindelin via GitGitGadget
> > > > <[e-mail]> wrote:
> > > >>         printf ".git\nfatal: Needed a single revision\n" >expect &&
> > > >> -       test_cmp expect output.txt
> > > >> +       sort <output.txt >output.sorted &&
> > > >> +       test_cmp expect output.sorted
> > > >
> > > > It was quite surprising to see this sorting only 'output' but not
> > > > 'expect'. I see now that 'output' is already "sorted" (in that sense),
> > > > but it feels fragile. More robust would be to sort 'expect' as well:
> > > >
> > > >     printf ".git\nfatal: Needed a single revision\n" | sort >expect &&
> > >
> > > Following Dscho's recent objection elsewhere that tests tend to check
> > > for much more than regressions, wouldn't it be logical to write these as
> > >
> > >         grep -F .git" output.txt &&
> > >         test_i18n_grep "Needed a single rev" output.txt
> > >
> > > without an 'expect' file at all?
> >
> > I considered suggesting that, as well, as being more obvious and less
> > fragile (with the exception that "Needed a single rev" isn't currently
> > localizable in builtin/rev-parse.c, so plain 'grep' instead of
> > 'test_i18n_grep').

Interesting side note: I just realized that t6050-replace.sh does indeed
contain

	test_i18ngrep "Needed a single revision" err

so I wonder why that works.

If anybody has an answer, I'd be curious, but for now I want to focus on
this here patch series instead.

Ciao,
Dscho

> Valid points all around, thank you so much!
>
> The next iteration will have the two `grep`s instead,
> Dscho
>

Johannes Sixt June 19, 2019, 5:24 p.m. UTC | #6

Am 19.06.19 um 13:30 schrieb Johannes Schindelin:
> Interesting side note: I just realized that t6050-replace.sh does indeed
> contain
> 
> 	test_i18ngrep "Needed a single revision" err
> 
> so I wonder why that works.

Why should it not work? If GIT_TEST_GETTEXT_POISON is on, it pretends
success; otherwise, it does a regular grep.

-- Hannes

Johannes Schindelin June 19, 2019, 8:58 p.m. UTC | #7

Hi Hannes,

On Wed, 19 Jun 2019, Johannes Sixt wrote:

> Am 19.06.19 um 13:30 schrieb Johannes Schindelin:
> > Interesting side note: I just realized that t6050-replace.sh does indeed
> > contain
> >
> > 	test_i18ngrep "Needed a single revision" err
> >
> > so I wonder why that works.
>
> Why should it not work? If GIT_TEST_GETTEXT_POISON is on, it pretends
> success; otherwise, it does a regular grep.

Ah, right, I vaguely remembered that Duy offered patches to do something
funny with a pseudo locale (I never understood why not go for the much
simpler ROT-13), and I guess that effort never went anywhere.

So essentially the test case in t6050 will succeed under GETTEXT_POISON
for the wrong reasons.

Thanks for explaining this to me!

Ciao,
Dscho

Patch

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 77a224aafb..c7ab18f40f 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -474,7 +474,8 @@  test_expect_success MINGW 'redirect std handles' '
 		GIT_REDIRECT_STDERR="2>&1" \
 		git rev-parse --git-dir --verify refs/invalid &&
 	printf ".git\nfatal: Needed a single revision\n" >expect &&
-	test_cmp expect output.txt
+	sort <output.txt >output.sorted &&
+	test_cmp expect output.sorted
 '
 
 test_done