| Message ID | 20230701192642.647167-1-martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] t0091-bugreport.sh: actually verify some content of report | expand |
Hi Martin On 01/07/2023 20:26, 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.41.0.2.gfb7d80edca: No such file or directory > > This doesn't disturb the test, which tugs along and reports success, not > really having verified the contents of the report at all. Thanks for the clear description of the problem and for fixing it > 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 split this test into three: first verify that we generate > a report at all, then check that the introductory blurb looks the way it > should, then verify that the "[System Info]" seems to contain the right > things. (The "[Enabled Hooks]" section is tested later in the script.) That sounds like a good plan > +test_expect_success 'report contains wanted template (before first section)' ' > + awk "/^\[/ { exit } { print }" git-bugreport-format.txt >actual && Personally I'd find sed -n -e '/^\[/q;p' git-bugreport-format.txt >actual easier to understand but that's probably because I don't use awk very much. I'm not sure it is worth a re-roll though as I see we do use awk in a few of the other test scripts. Best Wishes Phillip
Hi Phillip, On Mon, 3 Jul 2023 at 17:47, Phillip Wood <phillip.wood123@gmail.com> wrote: > > > +test_expect_success 'report contains wanted template (before first section)' ' > > + awk "/^\[/ { exit } { print }" git-bugreport-format.txt >actual && > > Personally I'd find > > sed -n -e '/^\[/q;p' git-bugreport-format.txt >actual > > easier to understand but that's probably because I don't use awk very > much. I'm not sure it is worth a re-roll though as I see we do use awk > in a few of the other test scripts. I do see we use sed quite a bit more than awk in the tests. This script doesn't yet use awk and if most feel like you we're probably better off introducing another sed use rather than another awk use. I'll be happy to post a v3 with only this change. (This script uses "sed -ne" already and I even do so elsewhere in my patch, so I'll be applying s/-n -e/-ne/ to your suggestion.) Thanks for reviewing! Martin
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh index b6d2f591ac..9390631b17 100755 --- a/t/t0091-bugreport.sh +++ b/t/t0091-bugreport.sh @@ -5,29 +5,50 @@ test_description='git bugreport' TEST_PASSES_SANITIZE_LEAK=true . ./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_when_finished rm git-bugreport-check-headers.txt && - git bugreport -s check-headers && - check_all_headers_populated <git-bugreport-check-headers.txt +test_expect_success 'create a report' ' + git bugreport -s format && + test_file_not_empty git-bugreport-format.txt +' + +test_expect_success 'report contains wanted template (before first section)' ' + awk "/^\[/ { exit } { print }" git-bugreport-format.txt >actual && + cat >expect <<-\EOF && + Thank you for filling out a Git bug report! + Please answer the following questions to help us understand your issue. + + What did you do before the bug happened? (Steps to reproduce your issue) + + What did you expect to happen? (Expected behavior) + + What happened instead? (Actual behavior) + + What'\''s different between what you expected and what actually happened? + + Anything else you want to add: + + Please review the rest of the bug report below. + You can delete any lines you don'\''t wish to share. + + + EOF + test_cmp expect actual +' + +test_expect_success 'sanity check "System Info" section' ' + test_when_finished rm -f git-bugreport-format.txt && + + sed -ne "/^\[System Info\]$/,/^$/p" <git-bugreport-format.txt >system && + + # The beginning should match "git version --build-info" verbatim, + # but rather than checking bit-for-bit equality, just test some basics. + grep "git version [0-9]." system && + grep "shell-path: ." system && + + # After the version, there should be some more info. + # This is bound to differ from environment to environment, + # so we just do some rather high-level checks. + grep "uname: ." system && + grep "compiler info: ." system ' 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.41.0.2.gfb7d80edca: No such file or directory 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 split this test into three: first verify that we generate a report at all, then check that the introductory blurb looks the way it should, then verify that the "[System Info]" seems to contain the right things. (The "[Enabled Hooks]" section is tested later in the script.) Reported-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- This is a much belated follow-up to v1 [1]. Feedback on that patch ranged from not bothering checking the generated report at all to implementing `--no-system-info` and `--no-hooks-info` so that we could easily test the introductory blurb verbatim. I hope I'm hitting a reasonable middle ground here. Verifying the contents at least somewhat is in line with the original intention of the test. Those `--no-...-info` could probably be useful to bug reporters, but it feels wrong to me to implement them just to be able to use them in the tests. [1] https://lore.kernel.org/git/20210411143354.25134-1-martin.agren@gmail.com/ t/t0091-bugreport.sh | 67 +++++++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 23 deletions(-)