Re: [PATCH v2 7/8] test-lib: test_region looks for trace2 regions

"Derrick Stolee via GitGitGadget" [off-list ref] writes:

... Note that this
changes the expectations slightly. The old test (incorrectly) used two
patterns for the 'grep' invocation, but this performs an OR of the
patterns, not an AND. This means that as long as one region_enter event
was logged, the test would succeed, even if it was not due to the
progress category.

 	# 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 progress "Working hard" trace.event &&

So we do want "region_enter" and the "category":"progress" on the
same line in the event file, but as long as "category":"progress"
exists, both will pass, regardless of enter/leave.  And ...

+test_region () {
+	local expect_exit=0
+	if test "$1" = "!"
+	then
+		expect_exit=1
+		shift
+	fi
+
+	grep -e "\"region_enter\".*\"category\":\"$1\",\"label\":\"$2\"" "$3"

... this makes sure there is enter/category on a line (and
leave/category on a line with another check).  Makes sense.

But...

	test_region '!' '\(unmatching capture)' 'two' 'three'

would try to use an invalid regexp and cause grep to exit with 2,
which would mean ...

+	exitcode=$?
+
+	if test $exitcode != $expect_exit

... this will not trigger and we return "success" (i.e. "failed as
expected")?

	Clarification.  The point is *NOT* that the grep pattern is
	not robust against funnies in $1 and $2---after all, these
	strings are under our control.  The point is what should
	happen when "grep" exits with an error when asked to ensure
	that there is no region detected.

+	then
+		return 1
+	fi
+
+	grep -e "\"region_leave\".*\"category\":\"$1\",\"label\":\"$2\"" "$3"

The same comment on "what about an error from grep" applies to this
one.

It might be easier to read to avoid having to say too many
backslash-quoted double quotes:

	grep -e	'"region_leave".*"category":"'"$1"'","label":"'"$2"\" "$3"

This comment applies to the earlier "grep", too.

+	exitcode=$?
+
+	if test $exitcode != $expect_exit
+	then
+		return 1
+	fi
+}

On 1/22/2021 2:42 PM, Junio C Hamano wrote:

"Derrick Stolee via GitGitGadget" [off-list ref] writes:

quoted

+	grep -e "\"region_enter\".*\"category\":\"$1\",\"label\":\"$2\"" "$3"

... this makes sure there is enter/category on a line (and
leave/category on a line with another check).  Makes sense.

But...

	test_region '!' '\(unmatching capture)' 'two' 'three'

would try to use an invalid regexp and cause grep to exit with 2,
which would mean ...

quoted

+	exitcode=$?
+
+	if test $exitcode != $expect_exit

... this will not trigger and we return "success" (i.e. "failed as
expected")?

Am I misunderstanding something here? If exitcode is 2, then this
will always trigger and return 1, signaling a failure. That would
propagate to the parent test and cause the test to fail. That seems
like the correct intention, but I'm not 100% confident about that.

	Clarification.  The point is *NOT* that the grep pattern is
	not robust against funnies in $1 and $2---after all, these
	strings are under our control.  The point is what should
	happen when "grep" exits with an error when asked to ensure
	that there is no region detected.

I'll be more robust to these in the next version. We'll expect
exit code equal to zero or _not_ equal to zero, depending on the
presence of '!'. This has the downside of returning success for
bad input strings when '!' is specified.

Basically, the approach I'm taking for v3 is here:

	if [test $expect_exit = 1] && [test $exitcode = 0]
	then
		return 1
	elif [test $expect_exit = 0] && [test $exitcode != 0]
	then
		return 1
	fi

quoted

+	grep -e "\"region_leave\".*\"category\":\"$1\",\"label\":\"$2\"" "$3"

The same comment on "what about an error from grep" applies to this
one.

It might be easier to read to avoid having to say too many
backslash-quoted double quotes:

	grep -e	'"region_leave".*"category":"'"$1"'","label":"'"$2"\" "$3"

This comment applies to the earlier "grep", too.

Thanks. This does look a bit cleaner.

-Stolee