Re: [PATCH] t0091-bugreport.sh: actually verify some content of report

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Apr 13, 2021 at 09:27:33PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Tue, Apr 13 2021, Martin Ågren wrote:
> 
> > On Mon, 12 Apr 2021 at 19:17, Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Martin Ågren <martin.agren@gmail.com> writes:
> >>
> >> > In the first test in this script, 'creates a report with content in the
> >> > right places', we generate a report and pipe it into our helper
> >> > `check_all_headers_populated()`. The idea of the helper is to find all
> >> > lines that look like headers ("[Some Header Here]") and to check that
> >> > the next line is non-empty. This is supposed to catch erroneous outputs
> >> > such as the following:
> > ...
> >> > Let's instead grep for some contents that we expect to find in a bug
> >> > report. We won't verify that they appear in the right order, but at
> >> > least we end up verifying the contents more than before this commit.
> >>
> >> Nicely described.  I agree that the original intent (let alone the
> >> implementation) is misguided and we should allow an empty section as
> >> a perfectly normal thing.
> >
> >> > +test_expect_success 'creates a report with content' '
> >> >       test_when_finished rm git-bugreport-check-headers.txt &&
> >> >       git bugreport -s check-headers &&
> >> > -     check_all_headers_populated <git-bugreport-check-headers.txt
> >> > +     grep "^Please answer " git-bugreport-check-headers.txt &&
> >> > +     grep "^\[System Info\]$" git-bugreport-check-headers.txt &&
> >> > +     grep "^git version:$" git-bugreport-check-headers.txt &&
> >> > +     grep "^\[Enabled Hooks\]$" git-bugreport-check-headers.txt
> >> >  '
> >>
> >> It is a different matter if it is sufficient to ensure only certain
> >> selected lines appear in the report, though.  As all the lines lost
> >> by this fix comes from 238b439d (bugreport: add tool to generate
> >> debugging info, 2020-04-16), it would be nice to hear from Emily.
> >
> > Maybe something like
> >
> >        awk '\''BEGIN { sect="" }
> >                /^\[.*]$/ { sect=$0 }
> >                /./ { print sect, $0 }'\'' \
> >            git-bugreport-check-headers.txt >prefixed &&
> >        grep "^ Thank you for filling out a Git bug report" prefixed &&
> >        grep "^ Please review the rest of the bug report below" prefixed &&
> >        grep "^ You can delete any lines you don.t wish to share" prefixed &&
> >        grep "\[System Info\] git version ..." prefixed
> >
> > Something like that could be used to verify that a line goes into the
> > right section, as opposed to just seeing that it appears *somewhere*. Or
> > maybe
> >
> >   grep -e Thank.you -e Please.review -e You.can.delete -e "^\[" \
> >        -e git.version git-bugreport-check-headers.txt >actual
> >
> > then setting up an "expect" and comparing. That would help us verify the
> > order, including which section things appear in. Slightly less friendly
> > for comparing loosely, compared to the awk-then-grep above.
> >
> > Let's see what Emily thinks about the various alternatives. Maybe she can
> > think of something else.

My apologies for the slow reply :)

> I think a straight-up test_cmp is preferrable, both for correctness and
> also as self-documentation, you can see from the test what the full
> expected output is like.

Yeah, I like the sound of this.

> 
> Obviously in this case we can't do a test_cmp on the raw output, as it
> contains various things from uname.
> 
> But it looks like we could do that if we do some light awk/perl/sed
> munging of the "[System Info]" and "[Enabled Hooks]" section(s).
> 
> Or, since we also control the generator we could pass a --no-system-info
> and/or --no-hooks-info, which may be something some people want for
> privacy/reporting reasons anyway (e.g. I've often used perlbug and
> deleted that whole info, because info there has no relevance to the
> specific issue I'm reporting).

This approach sounds more appealing to me than awk munging. I think
you're right that folks may not want to share it in some cases.

Thanks for noticing.
 - Emily