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

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

[Martin Ågren]

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>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t0091-bugreport.sh | 67 +++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index b6d2f591ac..f6998269be 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)' '
+	sed -ne "/^\[/q;p" 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' '
-- 
2.41.0.404.g5b50783d6b

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

[Martin Ågren]

On Wed, 5 Jul 2023 at 20:35, Martin Ågren <martin.agren@gmail.com> wrote:
>
> In the first test in this script, 'creates a report with content in the

This v3 has the wrong In-Reply-To. I've resent to the original thread,
https://lore.kernel.org/git/20230705184058.3057709-1-martin.agren@gmail.com/

Sorry about the noise,
Martin

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

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> +test_expect_success 'create a report' '
> +	git bugreport -s format &&
> +	test_file_not_empty git-bugreport-format.txt
> +'

OK.

> +test_expect_success 'report contains wanted template (before first section)' '
> +	sed -ne "/^\[/q;p" 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
> +'

I am not sure about the value of the bit-for-bit test here, but OK.
It is not like we will be changing this piece of text every week.

> +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.

Do you want to assert the "after" part?  "grep" alone does not do
anything of that sort.

> +	# 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
>  '

Don't we at least want to anchor all these patterns with "^" or
something?

Alternatively, since we do not expect the values of the fields are useful
at all, perhaps doing something like this

    sed -n -e '/^\[System Info\]/,/\[Enabled Hooks]/s/^\([^:]*):.*/\1/p' >names

to ensure that we have the fields we expect in the output makes more sense?

I notice that "git version:" does not have its value on its line.
Isn't it a bug we would rather fix before writing this "sanity check"
test, I have to wonder.

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I notice that "git version:" does not have its value on its line.
> Isn't it a bug we would rather fix before writing this "sanity check"
> test, I have to wonder.

 builtin/bugreport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/builtin/bugreport.c w/builtin/bugreport.c
index daf6c23657..af34dd6e1f 100644
--- c/builtin/bugreport.c
+++ w/builtin/bugreport.c
@@ -19,7 +19,7 @@ static void get_system_info(struct strbuf *sys_info)
 	char *shell = NULL;
 
 	/* get git version from native cmd */
-	strbuf_addstr(sys_info, _("git version:\n"));
+	strbuf_addstr(sys_info, _("git version: "));
 	get_version_info(sys_info, 1);
 
 	/* system call for other version info */

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

[Martin Ågren]

On Wed, 5 Jul 2023 at 21:53, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > +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.
>
> Do you want to assert the "after" part?  "grep" alone does not do
> anything of that sort.

Right, I remember thinking this 'after' could be useful to a human being
reading along, but that the test isn't enforcing it, as you point out.
If it's just misleading though, maybe we're better off either dropping
the "After" or enforcing it.

> > +     # 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
> >  '
>
> Don't we at least want to anchor all these patterns with "^" or
> something?
>
> Alternatively, since we do not expect the values of the fields are useful
> at all, perhaps doing something like this
>
>     sed -n -e '/^\[System Info\]/,/\[Enabled Hooks]/s/^\([^:]*):.*/\1/p' >names
>
> to ensure that we have the fields we expect in the output makes more sense?

The latter would make sense, yes. I could amend the patch to do
something like that, but I see you've already merged it to next. I'll
look into doing it as a patch on top during the weekend.

> I notice that "git version:" does not have its value on its line.
> Isn't it a bug we would rather fix before writing this "sanity check"
> test, I have to wonder.

Yeah, I noticed this. In my case, there's this:

 git version:
 git version 2.41.0.428.gbd0ef4c9fd
 cpu: x86_64
 built from commit: bd0ef4c9fd591e9c2ea1310dae92fe07a51438c7
 sizeof-long: 8
 sizeof-size_t: 8
 shell-path: /bin/sh
 uname: [...]
 [...]

IMHO, "git version: git version 2.41.0.428.gbd0ef4c9fd" would look sort
of weird. I tend to think "git version:" is more of a header for the
first several lines. It could perhaps be something like this, after
adding "--build-options" and indenting its output:

 git version --build-options:
   git version 2.41.0.428.gbd0ef4c9fd
   cpu: x86_64
   built from commit: bd0ef4c9fd591e9c2ea1310dae92fe07a51438c7
   sizeof-long: 8
   sizeof-size_t: 8
   shell-path: /bin/sh
 uname: [...]
 [...]

That's how I think of the contents. Actually changing the format would
be out-of-scope for the test updates, of course. I also wonder if there
are scripts out there trying to parse these reports and how they would
cope with such a tweak to the format.

Martin

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

[Martin Ågren]

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

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

[Martin Ågren]

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>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 (Resend of v3, now with correct In-Reply-To.)

 t/t0091-bugreport.sh | 67 +++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index b6d2f591ac..f6998269be 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)' '
+	sed -ne "/^\[/q;p" 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' '
-- 
2.41.0.404.g5b50783d6b

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

[Phillip Wood]

Hi Martin

This version looks good to me, thanks for re-rolling

Phillip

On 05/07/2023 19:40, 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.
> 
> 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>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>   (Resend of v3, now with correct In-Reply-To.)
> 
>   t/t0091-bugreport.sh | 67 +++++++++++++++++++++++++++++---------------
>   1 file changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> index b6d2f591ac..f6998269be 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)' '
> +	sed -ne "/^\[/q;p" 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' '