From: Emily Shaffer <emilyshaffer@google.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: "Martin Ågren" <martin.agren@gmail.com>,
"Junio C Hamano" <gitster@pobox.com>,
"SZEDER Gábor" <szeder.dev@gmail.com>,
"Git Mailing List" <git@vger.kernel.org>
Subject: Re: [PATCH] t0091-bugreport.sh: actually verify some content of report
Date: Tue, 13 Apr 2021 15:21:00 -0700 [thread overview]
Message-ID: <YHYZTLl90rkWWVOr@google.com> (raw)
In-Reply-To: <87a6q22dei.fsf@evledraar.gmail.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
next prev parent reply other threads:[~2021-04-13 22:21 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-16 21:18 [PATCH v13 0/5] git-bugreport with fixed VS build Emily Shaffer
2020-04-16 21:18 ` [PATCH v13 1/5] help: move list_config_help to builtin/help Emily Shaffer
2020-04-16 22:21 ` Junio C Hamano
2020-04-16 22:28 ` Junio C Hamano
2020-04-17 19:36 ` Emily Shaffer
2020-04-17 2:04 ` Danh Doan
2020-04-17 2:11 ` Danh Doan
2021-04-08 21:29 ` [PATCH] Makefile: add missing dependencies of 'config-list.h' SZEDER Gábor
2021-04-08 22:08 ` Ævar Arnfjörð Bjarmason
2021-04-08 23:40 ` Jeff King
2021-04-09 21:20 ` SZEDER Gábor
2021-04-16 19:03 ` Junio C Hamano
2021-04-16 21:33 ` SZEDER Gábor
2021-04-16 22:25 ` Junio C Hamano
2021-04-13 19:07 ` Ævar Arnfjörð Bjarmason
2020-04-16 21:18 ` [PATCH v13 2/5] bugreport: add tool to generate debugging info Emily Shaffer
2020-08-12 15:53 ` SZEDER Gábor
2021-04-08 21:36 ` SZEDER Gábor
2020-04-16 21:18 ` [PATCH v13 3/5] bugreport: gather git version and build info Emily Shaffer
2020-04-16 21:18 ` [PATCH v13 4/5] bugreport: add uname info Emily Shaffer
2021-04-08 22:19 ` Ævar Arnfjörð Bjarmason
2021-04-08 22:25 ` Junio C Hamano
2021-04-08 22:33 ` Ævar Arnfjörð Bjarmason
2021-04-08 23:41 ` Emily Shaffer
2021-04-08 23:58 ` Junio C Hamano
2021-04-09 21:27 ` SZEDER Gábor
2021-04-11 14:33 ` [PATCH] t0091-bugreport.sh: actually verify some content of report Martin Ågren
2021-04-12 17:17 ` Junio C Hamano
2021-04-13 18:32 ` Martin Ågren
2021-04-13 19:27 ` Ævar Arnfjörð Bjarmason
2021-04-13 22:21 ` Emily Shaffer [this message]
2023-07-01 19:26 ` [PATCH v2] " Martin Ågren
2023-07-03 15:47 ` Phillip Wood
2023-07-05 18:31 ` Martin Ågren
2023-07-05 18:40 ` [PATCH v3] " Martin Ågren
2023-07-05 19:46 ` Phillip Wood
2021-04-13 19:44 ` [PATCH] " SZEDER Gábor
2020-04-16 21:18 ` [PATCH v13 5/5] bugreport: add compiler info Emily Shaffer
2021-04-08 22:23 ` Ævar Arnfjörð Bjarmason
2021-04-08 22:59 ` Đoàn Trần Công Danh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YHYZTLl90rkWWVOr@google.com \
--to=emilyshaffer@google.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=martin.agren@gmail.com \
--cc=szeder.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).