diff mbox series

[8/9] test-lib: test_region looks for trace2 regions

Message ID 8326a9b5320e1e774caef568fcce2bfd2ec13cb1.1611161639.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series More index cleanups | expand

Commit Message

Derrick Stolee Jan. 20, 2021, 4:53 p.m. UTC
From: Derrick Stolee <dstolee@microsoft.com>

Most test cases can verify Git's behavior using input/output
expectations or changes to the .git directory. However, sometimes we
want to check that Git did or did not run a certain section of code.
This is particularly important for performance-only features that we
want to ensure have been enabled in certain cases.

Add a new 'test_region' function that checks if a trace2 region was
entered and left in a given trace2 event log.

There is one existing test (t0500-progress-display.sh) that performs
this check already, so use the helper function instead. More uses will
be added in a later change.

t6423-merge-rename-directories.sh also greps for region_enter lines, but
it verifies the number of such lines, which is not the same as an
existence check.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t0500-progress-display.sh |  3 +--
 t/test-lib-functions.sh     | 40 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

Comments

Elijah Newren Jan. 20, 2021, 6:20 p.m. UTC | #1
On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Most test cases can verify Git's behavior using input/output
> expectations or changes to the .git directory. However, sometimes we
> want to check that Git did or did not run a certain section of code.
> This is particularly important for performance-only features that we
> want to ensure have been enabled in certain cases.
>
> Add a new 'test_region' function that checks if a trace2 region was
> entered and left in a given trace2 event log.

Ooh, others do this too?  Sounds like a helpful function to add, but
just checking for entered and left means that...

>
> There is one existing test (t0500-progress-display.sh) that performs
> this check already, so use the helper function instead. More uses will
> be added in a later change.
>
> t6423-merge-rename-directories.sh also greps for region_enter lines, but
> it verifies the number of such lines, which is not the same as an
> existence check.

...yeah, won't cover the case that I added.  That's fine, since it
appears to be a one-off for now and we don't know of any other cases,
current or planned, that want to do something like that yet.

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t0500-progress-display.sh |  3 +--
>  t/test-lib-functions.sh     | 40 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
> index 1ed1df351cb..c461b89dfaf 100755
> --- a/t/t0500-progress-display.sh
> +++ b/t/t0500-progress-display.sh
> @@ -303,8 +303,7 @@ test_expect_success 'progress generates traces' '
>                 "Working hard" <in 2>stderr &&
>
>         # t0212/parse_events.perl intentionally omits regions and data.
> -       grep -e "region_enter" -e "\"category\":\"progress\"" trace.event &&
> -       grep -e "region_leave" -e "\"category\":\"progress\"" trace.event &&
> +       test_region category progress trace.event &&

Sidenote: Hmm...about 40% of my region labels in merge-ort.c and 90%
in diffcore-rename.c have spaces in them.  This function could still
be used, but I'm curious if I should change the labels (but then
again, they are testing logical regions rather than individual
functions, and the spaces instead of underscores kind of convey
that...)

>         grep "\"key\":\"total_objects\",\"value\":\"40\"" trace.event &&
>         grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event
>  '
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 999982fe4a9..c878db93013 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1655,3 +1655,43 @@ test_subcommand () {
>                 grep "\[$expr\]"
>         fi
>  }
> +
> +# Check that the given command was invoked as part of the
> +# trace2-format trace on stdin.
> +#
> +#      test_region [!] <category> <label> git <command> <args>...
> +#
> +# For example, to look for trace2_region_enter("index", "do_read_index", repo)
> +# in an invocation of "git checkout HEAD~1", run
> +#
> +#      GIT_TRACE2_EVENT="$(pwd)/trace.txt" GIT_TRACE2_EVENT_NESTING=10 \
> +#              git checkout HEAD~1 &&
> +#      test_region index do_read_index <trace.txt
> +#
> +# If the first parameter passed is !, this instead checks that
> +# the given region was not entered.
> +#
> +test_region () {
> +       local expect_exit=0
> +       if test "$1" = "!"
> +       then
> +               expect_exit=1
> +               shift
> +       fi
> +
> +       grep -e "region_enter" -e "\"category\":\"$1\",\"label\":\"$2\"" "$3"
> +       exitcode=$?
> +
> +       if test $exitcode != $expect_exit
> +       then
> +               return 1
> +       fi
> +
> +       grep -e "region_leave" -e "\"category\":\"$1\",\"label\":\"$2\"" "$3"
> +       exitcode=$?
> +
> +       if test $exitcode != $expect_exit
> +       then
> +               return 1
> +       fi
> +}
> --
> gitgitgadget

This patch looks good to me.
Derrick Stolee Jan. 20, 2021, 7:24 p.m. UTC | #2
On 1/20/2021 1:20 PM, Elijah Newren wrote:
> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
...
>>         # t0212/parse_events.perl intentionally omits regions and data.
>> -       grep -e "region_enter" -e "\"category\":\"progress\"" trace.event &&
>> -       grep -e "region_leave" -e "\"category\":\"progress\"" trace.event &&
>> +       test_region category progress trace.event &&
> 
> Sidenote: Hmm...about 40% of my region labels in merge-ort.c and 90%
> in diffcore-rename.c have spaces in them.  This function could still
> be used, but I'm curious if I should change the labels (but then
> again, they are testing logical regions rather than individual
> functions, and the spaces instead of underscores kind of convey
> that...)

You should be able to use

	test_region "category with spaces" "progress with spaces" trace

but if not, then the test_region helper could be improved to match.

I do think that it's better to avoid spaces in these identifiers.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 1ed1df351cb..c461b89dfaf 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -303,8 +303,7 @@  test_expect_success 'progress generates traces' '
 		"Working hard" <in 2>stderr &&
 
 	# t0212/parse_events.perl intentionally omits regions and data.
-	grep -e "region_enter" -e "\"category\":\"progress\"" trace.event &&
-	grep -e "region_leave" -e "\"category\":\"progress\"" trace.event &&
+	test_region category progress trace.event &&
 	grep "\"key\":\"total_objects\",\"value\":\"40\"" trace.event &&
 	grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event
 '
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 999982fe4a9..c878db93013 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1655,3 +1655,43 @@  test_subcommand () {
 		grep "\[$expr\]"
 	fi
 }
+
+# Check that the given command was invoked as part of the
+# trace2-format trace on stdin.
+#
+#	test_region [!] <category> <label> git <command> <args>...
+#
+# For example, to look for trace2_region_enter("index", "do_read_index", repo)
+# in an invocation of "git checkout HEAD~1", run
+#
+#	GIT_TRACE2_EVENT="$(pwd)/trace.txt" GIT_TRACE2_EVENT_NESTING=10 \
+#		git checkout HEAD~1 &&
+#	test_region index do_read_index <trace.txt
+#
+# If the first parameter passed is !, this instead checks that
+# the given region was not entered.
+#
+test_region () {
+	local expect_exit=0
+	if test "$1" = "!"
+	then
+		expect_exit=1
+		shift
+	fi
+
+	grep -e "region_enter" -e "\"category\":\"$1\",\"label\":\"$2\"" "$3"
+	exitcode=$?
+
+	if test $exitcode != $expect_exit
+	then
+		return 1
+	fi
+
+	grep -e "region_leave" -e "\"category\":\"$1\",\"label\":\"$2\"" "$3"
+	exitcode=$?
+
+	if test $exitcode != $expect_exit
+	then
+		return 1
+	fi
+}