{"thread":{"id":"59953","subject":"[PATCH v3] t0091-bugreport.sh: actually verify some content of report","startedAt":"2023-07-05T18:35:53Z","lastAt":"2023-07-07T12:26:50Z","messageCount":5,"participants":["Martin Ågren","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"479225","messageId":"20230705183532.3057433-1-martin.agren@gmail.com","threadId":"59953","inReplyTo":"AN0heSrMCnygWUC5Sh1UA9v2JGtjcxYDKPFE0xUPddGEW29c3w@mail.gmail.com","subject":"[PATCH v3] t0091-bugreport.sh: actually verify some content of report","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2023-07-05T18:35:27Z","receivedAt":"2023-07-05T18:35:53Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In the first test in this script, 'creates a report with content in the\nright places', we generate a report and pipe it into our helper\n`check_all_headers_populated()`. The idea of the helper is to find all\nlines that look like headers (\"[Some Header Here]\") and to check that\nthe next line is non-empty. This is supposed to catch erroneous outputs\nsuch as the following:\n\n [A Header]\n something\n more here\n\n [Another Header]\n\n [Too Early Header]\n contents\n\nHowever, we provide the lines of the bug report as filenames to grep,\nmeaning we mostly end up spewing errors:\n\n grep: : No such file or directory\n grep: [System Info]: No such file or directory\n grep: git version:: No such file or directory\n grep: git version 2.41.0.2.gfb7d80edca: No such file or directory\n\nThis doesn't disturb the test, which tugs along and reports success, not\nreally having verified the contents of the report at all.\n\nNote that after 788a776069 (\"bugreport: collect list of populated\nhooks\", 2020-05-07), the bug report, which is created in our hook-less\ntest repo, contains an empty section with the enabled hooks. Thus, even\nthe intention of our helper is a bit misguided: there is nothing\ninherently wrong with having an empty section in the bug report.\n\nLet's instead split this test into three: first verify that we generate\na report at all, then check that the introductory blurb looks the way it\nshould, then verify that the \"[System Info]\" seems to contain the right\nthings. (The \"[Enabled Hooks]\" section is tested later in the script.)\n\nReported-by: SZEDER Gábor \nHelped-by: Phillip Wood \nSigned-off-by: Martin Ågren \n---\n t/t0091-bugreport.sh | 67 +++++++++++++++++++++++++++++---------------\n 1 file changed, 44 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh\nindex b6d2f591ac..f6998269be 100755\n--- a/t/t0091-bugreport.sh\n+++ b/t/t0091-bugreport.sh\n@@ -5,29 +5,50 @@ test_description='git bugreport'\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n-# Headers \"[System Info]\" will be followed by a non-empty line if we put some\n-# information there; we can make sure all our headers were followed by some\n-# information to check if the command was successful.\n-HEADER_PATTERN=\"^\\[.*\\]$\"\n-\n-check_all_headers_populated () {\n-\twhile read -r line\n-\tdo\n-\t\tif test \"$(grep \"$HEADER_PATTERN\" \"$line\")\"\n-\t\tthen\n-\t\t\techo \"$line\"\n-\t\t\tread -r nextline\n-\t\t\tif test -z \"$nextline\"; then\n-\t\t\t\treturn 1;\n-\t\t\tfi\n-\t\tfi\n-\tdone\n-}\n-\n-test_expect_success 'creates a report with content in the right places' '\n-\ttest_when_finished rm git-bugreport-check-headers.txt &&\n-\tgit bugreport -s check-headers &&\n-\tcheck_all_headers_populated actual &&\n+\tcat >expect <<-\\EOF &&\n+\tThank you for filling out a Git bug report!\n+\tPlease answer the following questions to help us understand your issue.\n+\n+\tWhat did you do before the bug happened? (Steps to reproduce your issue)\n+\n+\tWhat did you expect to happen? (Expected behavior)\n+\n+\tWhat happened instead? (Actual behavior)\n+\n+\tWhat'\\''s different between what you expected and what actually happened?\n+\n+\tAnything else you want to add:\n+\n+\tPlease review the rest of the bug report below.\n+\tYou can delete any lines you don'\\''t wish to share.\n+\n+\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'sanity check \"System Info\" section' '\n+\ttest_when_finished rm -f git-bugreport-format.txt &&\n+\n+\tsed -ne \"/^\\[System Info\\]$/,/^$/p\" system &&\n+\n+\t# The beginning should match \"git version --build-info\" verbatim,\n+\t# but rather than checking bit-for-bit equality, just test some basics.\n+\tgrep \"git version [0-9].\" system &&\n+\tgrep \"shell-path: .\" system &&\n+\n+\t# After the version, there should be some more info.\n+\t# This is bound to differ from environment to environment,\n+\t# so we just do some rather high-level checks.\n+\tgrep \"uname: .\" system &&\n+\tgrep \"compiler info: .\" system\n '\n \n test_expect_success 'dies if file with same name as report already exists' '\n-- \n2.41.0.404.g5b50783d6b\n\n"},{"id":"479227","messageId":"CAN0heSqkg=_BKyPP1A-KMUR5+JBLY-HQxzfbUC4EjTjYX0pcHQ@mail.gmail.com","threadId":"59953","inReplyTo":"20230705183532.3057433-1-martin.agren@gmail.com","subject":"Re: [PATCH v3] t0091-bugreport.sh: actually verify some content of report","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2023-07-05T18:43:59Z","receivedAt":"2023-07-05T18:44:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 5 Jul 2023 at 20:35, Martin Ågren wrote:\n>\n> In the first test in this script, 'creates a report with content in the\n\nThis v3 has the wrong In-Reply-To. I've resent to the original thread,\nhttps://lore.kernel.org/git/20230705184058.3057709-1-martin.agren@gmail.com/\n\nSorry about the noise,\nMartin\n"},{"id":"479236","messageId":"xmqqv8eyyw2g.fsf@gitster.g","threadId":"59953","inReplyTo":"20230705183532.3057433-1-martin.agren@gmail.com","subject":"Re: [PATCH v3] t0091-bugreport.sh: actually verify some content of report","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T19:53:11Z","receivedAt":"2023-07-05T19:53:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> +test_expect_success 'create a report' '\n> +\tgit bugreport -s format &&\n> +\ttest_file_not_empty git-bugreport-format.txt\n> +'\n\nOK.\n\n> +test_expect_success 'report contains wanted template (before first section)' '\n> +\tsed -ne \"/^\\[/q;p\" git-bugreport-format.txt >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tThank you for filling out a Git bug report!\n> +\tPlease answer the following questions to help us understand your issue.\n> +\n> +\tWhat did you do before the bug happened? (Steps to reproduce your issue)\n> +\n> +\tWhat did you expect to happen? (Expected behavior)\n> +\n> +\tWhat happened instead? (Actual behavior)\n> +\n> +\tWhat'\\''s different between what you expected and what actually happened?\n> +\n> +\tAnything else you want to add:\n> +\n> +\tPlease review the rest of the bug report below.\n> +\tYou can delete any lines you don'\\''t wish to share.\n> +\n> +\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n\nI am not sure about the value of the bit-for-bit test here, but OK.\nIt is not like we will be changing this piece of text every week.\n\n> +test_expect_success 'sanity check \"System Info\" section' '\n> +\ttest_when_finished rm -f git-bugreport-format.txt &&\n> +\n> +\tsed -ne \"/^\\[System Info\\]$/,/^$/p\" system &&\n> +\n> +\t# The beginning should match \"git version --build-info\" verbatim,\n> +\t# but rather than checking bit-for-bit equality, just test some basics.\n> +\tgrep \"git version [0-9].\" system &&\n> +\tgrep \"shell-path: .\" system &&\n> +\n> +\t# After the version, there should be some more info.\n\nDo you want to assert the \"after\" part? \"grep\" alone does not do\nanything of that sort.\n\n> +\t# This is bound to differ from environment to environment,\n> +\t# so we just do some rather high-level checks.\n> +\tgrep \"uname: .\" system &&\n> +\tgrep \"compiler info: .\" system\n> '\n\nDon't we at least want to anchor all these patterns with \"^\" or\nsomething?\n\nAlternatively, since we do not expect the values of the fields are useful\nat all, perhaps doing something like this\n\n sed -n -e '/^\\[System Info\\]/,/\\[Enabled Hooks]/s/^\\([^:]*):.*/\\1/p' >names\n\nto ensure that we have the fields we expect in the output makes more sense?\n\nI notice that \"git version:\" does not have its value on its line.\nIsn't it a bug we would rather fix before writing this \"sanity check\"\ntest, I have to wonder.\n\nThanks.\n"},{"id":"479237","messageId":"xmqqr0pmyw0b.fsf@gitster.g","threadId":"59953","inReplyTo":"xmqqv8eyyw2g.fsf@gitster.g","subject":"Re: [PATCH v3] t0091-bugreport.sh: actually verify some content of report","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-05T19:54:28Z","receivedAt":"2023-07-05T19:54:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I notice that \"git version:\" does not have its value on its line.\n> Isn't it a bug we would rather fix before writing this \"sanity check\"\n> test, I have to wonder.\n\n builtin/bugreport.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git c/builtin/bugreport.c w/builtin/bugreport.c\nindex daf6c23657..af34dd6e1f 100644\n--- c/builtin/bugreport.c\n+++ w/builtin/bugreport.c\n@@ -19,7 +19,7 @@ static void get_system_info(struct strbuf *sys_info)\n \tchar *shell = NULL;\n \n \t/* get git version from native cmd */\n-\tstrbuf_addstr(sys_info, _(\"git version:\\n\"));\n+\tstrbuf_addstr(sys_info, _(\"git version: \"));\n \tget_version_info(sys_info, 1);\n \n \t/* system call for other version info */\n"},{"id":"479277","messageId":"CAN0heSpY-vYVFznq9YLtjj0Enmf957ASCLdN_Eyz0ZLKd82FWg@mail.gmail.com","threadId":"59953","inReplyTo":"xmqqv8eyyw2g.fsf@gitster.g","subject":"Re: [PATCH v3] t0091-bugreport.sh: actually verify some content of report","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2023-07-07T12:26:33Z","receivedAt":"2023-07-07T12:26:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 5 Jul 2023 at 21:53, Junio C Hamano wrote:\n>\n> Martin Ågren writes:\n>\n> > +test_expect_success 'sanity check \"System Info\" section' '\n> > + test_when_finished rm -f git-bugreport-format.txt &&\n> > +\n> > + sed -ne \"/^\\[System Info\\]$/,/^$/p\" system &&\n> > +\n> > + # The beginning should match \"git version --build-info\" verbatim,\n> > + # but rather than checking bit-for-bit equality, just test some basics.\n> > + grep \"git version [0-9].\" system &&\n> > + grep \"shell-path: .\" system &&\n> > +\n> > + # After the version, there should be some more info.\n>\n> Do you want to assert the \"after\" part? \"grep\" alone does not do\n> anything of that sort.\n\nRight, I remember thinking this 'after' could be useful to a human being\nreading along, but that the test isn't enforcing it, as you point out.\nIf it's just misleading though, maybe we're better off either dropping\nthe \"After\" or enforcing it.\n\n> > + # This is bound to differ from environment to environment,\n> > + # so we just do some rather high-level checks.\n> > + grep \"uname: .\" system &&\n> > + grep \"compiler info: .\" system\n> > '\n>\n> Don't we at least want to anchor all these patterns with \"^\" or\n> something?\n>\n> Alternatively, since we do not expect the values of the fields are useful\n> at all, perhaps doing something like this\n>\n> sed -n -e '/^\\[System Info\\]/,/\\[Enabled Hooks]/s/^\\([^:]*):.*/\\1/p' >names\n>\n> to ensure that we have the fields we expect in the output makes more sense?\n\nThe latter would make sense, yes. I could amend the patch to do\nsomething like that, but I see you've already merged it to next. I'll\nlook into doing it as a patch on top during the weekend.\n\n> I notice that \"git version:\" does not have its value on its line.\n> Isn't it a bug we would rather fix before writing this \"sanity check\"\n> test, I have to wonder.\n\nYeah, I noticed this. In my case, there's this:\n\n git version:\n git version 2.41.0.428.gbd0ef4c9fd\n cpu: x86_64\n built from commit: bd0ef4c9fd591e9c2ea1310dae92fe07a51438c7\n sizeof-long: 8\n sizeof-size_t: 8\n shell-path: /bin/sh\n uname: [...]\n [...]\n\nIMHO, \"git version: git version 2.41.0.428.gbd0ef4c9fd\" would look sort\nof weird. I tend to think \"git version:\" is more of a header for the\nfirst several lines. It could perhaps be something like this, after\nadding \"--build-options\" and indenting its output:\n\n git version --build-options:\n git version 2.41.0.428.gbd0ef4c9fd\n cpu: x86_64\n built from commit: bd0ef4c9fd591e9c2ea1310dae92fe07a51438c7\n sizeof-long: 8\n sizeof-size_t: 8\n shell-path: /bin/sh\n uname: [...]\n [...]\n\nThat's how I think of the contents. Actually changing the format would\nbe out-of-scope for the test updates, of course. I also wonder if there\nare scripts out there trying to parse these reports and how they would\ncope with such a tweak to the format.\n\nMartin\n"}]}