| Message ID | 20210411143354.25134-1-martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t0091-bugreport.sh: actually verify some content of report | expand |
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: > > [A Header] > something > more here > > [Another Header] > > [Too Early Header] > contents > > However, we provide the lines of the bug report as filenames to grep, > meaning we mostly end up spewing errors: > > grep: : No such file or directory > grep: [System Info]: No such file or directory > grep: git version:: No such file or directory > grep: git version 2.31.1.164.g984c2561cd: No such file > > This doesn't disturb the test, which tugs along and reports success, not > really having verified the contents of the report at all. > > Note that after 788a776069 ("bugreport: collect list of populated > hooks", 2020-05-07), the bug report, which is created in our hook-less > test repo, contains an empty section with the enabled hooks. Thus, even > the intention of our helper is a bit misguided: there is nothing > inherently wrong with having an empty section in the bug report. > > 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. > Reported-by: SZEDER Gábor <szeder.dev@gmail.com> > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > > It does scare me.. > > Maybe something like this? > ... > +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. Thanks.
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. Martin
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. 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. 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).
On Sun, Apr 11, 2021 at 04:33:54PM +0200, Martin Ågren wrote: > 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: > > [A Header] > something > more here > > [Another Header] > > [Too Early Header] > contents > > However, we provide the lines of the bug report as filenames to grep, > meaning we mostly end up spewing errors: > > grep: : No such file or directory > grep: [System Info]: No such file or directory > grep: git version:: No such file or directory > grep: git version 2.31.1.164.g984c2561cd: No such file > > This doesn't disturb the test, which tugs along and reports success, not > really having verified the contents of the report at all. > > Note that after 788a776069 ("bugreport: collect list of populated > hooks", 2020-05-07), the bug report, which is created in our hook-less > test repo, contains an empty section with the enabled hooks. Thus, even > the intention of our helper is a bit misguided: there is nothing > inherently wrong with having an empty section in the bug report. > > 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. > > Reported-by: SZEDER Gábor <szeder.dev@gmail.com> > Signed-off-by: Martin Ågren <martin.agren@gmail.com> > --- > > It does scare me.. > > Maybe something like this? Thanks! > t/t0091-bugreport.sh | 26 +++++--------------------- > 1 file changed, 5 insertions(+), 21 deletions(-) > > diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh > index 526304ff95..9111c4c26f 100755 > --- a/t/t0091-bugreport.sh > +++ b/t/t0091-bugreport.sh > @@ -4,29 +4,13 @@ test_description='git bugreport' > > . ./test-lib.sh > > -# Headers "[System Info]" will be followed by a non-empty line if we put some > -# information there; we can make sure all our headers were followed by some > -# information to check if the command was successful. > -HEADER_PATTERN="^\[.*\]$" > - > -check_all_headers_populated () { > - while read -r line > - do > - if test "$(grep "$HEADER_PATTERN" "$line")" > - then > - echo "$line" > - read -r nextline > - if test -z "$nextline"; then > - return 1; > - fi > - fi > - done > -} > - > -test_expect_success 'creates a report with content in the right places' ' > +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 && This "Please answer" is translated and you look for it with plain 'grep' instead of 'test_i18ngrep', which is fine nowadays... However, Junio queued this patch on top of v2.29.3, which is old enough to still have the GETTEXT_POISON CI job, and fails because of this. > + grep "^\[System Info\]$" git-bugreport-check-headers.txt && > + grep "^git version:$" git-bugreport-check-headers.txt && > + grep "^\[Enabled Hooks\]$" git-bugreport-check-headers.txt > ' I have to wonder, however, whether this is worth testing at all. > > test_expect_success 'dies if file with same name as report already exists' ' > -- > 2.31.1.163.ga65ce7f831 >
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
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh index 526304ff95..9111c4c26f 100755 --- a/t/t0091-bugreport.sh +++ b/t/t0091-bugreport.sh @@ -4,29 +4,13 @@ test_description='git bugreport' . ./test-lib.sh -# Headers "[System Info]" will be followed by a non-empty line if we put some -# information there; we can make sure all our headers were followed by some -# information to check if the command was successful. -HEADER_PATTERN="^\[.*\]$" - -check_all_headers_populated () { - while read -r line - do - if test "$(grep "$HEADER_PATTERN" "$line")" - then - echo "$line" - read -r nextline - if test -z "$nextline"; then - return 1; - fi - fi - done -} - -test_expect_success 'creates a report with content in the right places' ' +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 ' test_expect_success 'dies if file with same name as report already exists' '
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: [A Header] something more here [Another Header] [Too Early Header] contents However, we provide the lines of the bug report as filenames to grep, meaning we mostly end up spewing errors: grep: : No such file or directory grep: [System Info]: No such file or directory grep: git version:: No such file or directory grep: git version 2.31.1.164.g984c2561cd: No such file This doesn't disturb the test, which tugs along and reports success, not really having verified the contents of the report at all. Note that after 788a776069 ("bugreport: collect list of populated hooks", 2020-05-07), the bug report, which is created in our hook-less test repo, contains an empty section with the enabled hooks. Thus, even the intention of our helper is a bit misguided: there is nothing inherently wrong with having an empty section in the bug report. 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. Reported-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- > It does scare me.. Maybe something like this? t/t0091-bugreport.sh | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-)