threads / rfc / 58999

RFC patchtest-lib: allow storing counts with test harnesses

Subject: [RFC PATCH] test-lib: allow storing counts with test harnesses

## tl;dr

5 messages between Dec 24, 2022 and Apr 1, 2023. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

Adam Dinwoodie· Dec 24, 2022, 22:52 UTC · lore

Currently, test result files are only stored in test-results/*.counts if $HARNESS_ACTIVE is not set. This dates from 8ef1abe550 (test-lib: Don't write test-results when HARNESS_ACTIVE, 2010-08-11), where the assumption was that if someone were using a test harness like prove, that would track results and the count files wouldn't be required. However, as of 49da404070 (test-lib: show missing prereq summary, 2021-11-20), those files also store the list of git test prerequisites that were missing during the test run, which isn't something that a generic test harness like prove can provide.

To allow folk using test harnesses to access the lists of missing prerequisites, add a --counts argument to test-lib that will keep these counts files even if a test harness is in use. This means that a subsequent call of, say, `make -C t aggregate-results` will report useful information.

It might be preferable to do make a wider-ranging change, including storing the missing prerequisites separately from the count files, so the results can be reported regardless of whether the success/failure counts are wanted, but that would be more disruptive and more work for relatively little gain.

Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
---

The key reason I'm submitting this as an RFC is that last paragraph: I've tested the below patch, and it achieves what I'm after (letting me both use prove and audit missing prerequisites), but it further embeds the use of the "count" files for recording missing prerequisites, where it might be preferable to do something a bit more complex that treats the prerequisite reporting as a separate function, rather than a bolt-on to the pass/fail/etc. counts.

 t/test-lib.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to t/test-lib.sh +3 −1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 6db377f68b..bbd9ee0e34 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -157,6 +157,8 @@ parse_option () {
 	local opt="$1"
 
 	case "$opt" in
+	-c|--c|--co|--cou|--coun|--count|--counts)
+		record_counts=t ;;
 	-d|--d|--de|--deb|--debu|--debug)
 		debug=t ;;
 	-i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
@@ -1282,7 +1284,7 @@ test_done () {
 
 	finalize_test_output
 
-	if test -z "$HARNESS_ACTIVE"
+	if test -z "$HARNESS_ACTIVE" || test -n "$record_counts"
 	then
 		mkdir -p "$TEST_RESULTS_DIR"
 

-- 
2.39.0
Adam Dinwoodie· Mar 4, 2023, 21:22 UTC · re: Adam Dinwoodie · lore

[PATCH] test-lib: allow storing counts with test harnesses

Currently, test result files are only stored in test-results/*.counts if $HARNESS_ACTIVE is not set. This dates from 8ef1abe550 (test-lib: Don't write test-results when HARNESS_ACTIVE, 2010-08-11), where the assumption was that if someone were using a test harness like prove, that would track results and the count files wouldn't be required. However, as of 49da404070 (test-lib: show missing prereq summary, 2021-11-20), those files also store the list of git test prerequisites that were missing during the test run, which isn't something that a generic test harness like prove can provide.

To allow folk using test harnesses to access the lists of missing prerequisites, add a --counts argument to test-lib that will keep these counts files even if a test harness is in use. This means that a subsequent call of, say, `make -C t aggregate-results` will report useful information.

It might be preferable to do make a wider-ranging change, including storing the missing prerequisites separately from the count files, so the results can be reported regardless of whether the success/failure counts are wanted, but that would be more disruptive and more work for relatively little gain.

Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
---

I submitted this as an RFC back in December, and received no comments, so I'm submitting this as an actual patch now. My key concern was the final paragraph above -- embedding using the "count" files for something other than counts -- but I've mostly convinced myself that refactoring this code to separate that out is unlikely to actually cause significant pain.

 t/test-lib.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to t/test-lib.sh +3 −1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 6db377f68b..bbd9ee0e34 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -157,6 +157,8 @@ parse_option () {
 	local opt="$1"
 
 	case "$opt" in
+	-c|--c|--co|--cou|--coun|--count|--counts)
+		record_counts=t ;;
 	-d|--d|--de|--deb|--debu|--debug)
 		debug=t ;;
 	-i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
@@ -1282,7 +1284,7 @@ test_done () {
 
 	finalize_test_output
 
-	if test -z "$HARNESS_ACTIVE"
+	if test -z "$HARNESS_ACTIVE" || test -n "$record_counts"
 	then
 		mkdir -p "$TEST_RESULTS_DIR"
Jeff King· Mar 6, 2023, 08:57 UTC · re: Adam Dinwoodie · lore

Re: [PATCH] test-lib: allow storing counts with test harnesses

On Sat, Mar 04, 2023 at 09:22:20PM +0000, Adam Dinwoodie wrote:
Show 15 quoted lines
> Currently, test result files are only stored in test-results/*.counts if
> $HARNESS_ACTIVE is not set.  This dates from 8ef1abe550 (test-lib: Don't
> write test-results when HARNESS_ACTIVE, 2010-08-11), where the
> assumption was that if someone were using a test harness like prove,
> that would track results and the count files wouldn't be required.
> However, as of 49da404070 (test-lib: show missing prereq summary,
> 2021-11-20), those files also store the list of git test prerequisites
> that were missing during the test run, which isn't something that a
> generic test harness like prove can provide.
> 
> To allow folk using test harnesses to access the lists of missing
> prerequisites, add a --counts argument to test-lib that will keep these
> counts files even if a test harness is in use.  This means that a
> subsequent call of, say, `make -C t aggregate-results` will report
> useful information.

Your goal seems reasonable. I have to wonder if it is even worth requiring "--counts" here, though. Even 8ef1abe550 claims that the I/O from writing the results files is minimal. And certainly I run under prove with "--root=/some/ram/disk", and I haven't noticed any difference with and without my usual "--verbose-log", which writes a lot more data into test-results/.

So would it be worth it to just revert 8ef1abe550, and always store the meta-files? That's one less option to support, and one less surprise when some other feature is built around them.

Or is there some reason that we really want to have a mode where nothing is written into t/? From reading 8ef1abe550 it sounded like this was mostly a hygiene / optimization thing, and not some special mode we cared about supporting.

>  t/test-lib.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
The patch itself looks correct, if we want to go with a --counts option.
-Peff
Junio C Hamano· Mar 6, 2023, 18:15 UTC · re: Jeff King · lore

Re: [PATCH] test-lib: allow storing counts with test harnesses

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> So would it be worth it to just revert 8ef1abe550, and always store the
> meta-files? That's one less option to support, and one less surprise
> when some other feature is built around them.
>
> Or is there some reason that we really want to have a mode where nothing
> is written into t/? From reading 8ef1abe550 it sounded like this was
> mostly a hygiene / optimization thing, and not some special mode we
> cared about supporting.

Interesting thought. It would simplify our lives to have fewer conditionally-moving parts.

>>  t/test-lib.sh | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> The patch itself looks correct, if we want to go with a --counts option.
Yes.
Elijah Newren· Apr 1, 2023, 18:56 UTC · re: Adam Dinwoodie · lore

Re: [PATCH] test-lib: allow storing counts with test harnesses

On Sat, Mar 4, 2023 at 2:16 PM Adam Dinwoodie <adam@dinwoodie.org> wrote:
Show 18 quoted lines
>
> Currently, test result files are only stored in test-results/*.counts if
> $HARNESS_ACTIVE is not set.  This dates from 8ef1abe550 (test-lib: Don't
> write test-results when HARNESS_ACTIVE, 2010-08-11), where the
> assumption was that if someone were using a test harness like prove,
> that would track results and the count files wouldn't be required.
> However, as of 49da404070 (test-lib: show missing prereq summary,
> 2021-11-20), those files also store the list of git test prerequisites
> that were missing during the test run, which isn't something that a
> generic test harness like prove can provide.
>
> To allow folk using test harnesses to access the lists of missing
> prerequisites, add a --counts argument to test-lib that will keep these
> counts files even if a test harness is in use.  This means that a
> subsequent call of, say, `make -C t aggregate-results` will report
> useful information.
>
> It might be preferable to do make a wider-ranging change, including
Replace "do make" with either "do" or "make"?
Show 14 quoted lines
> storing the missing prerequisites separately from the count files, so
> the results can be reported regardless of whether the success/failure
> counts are wanted, but that would be more disruptive and more work for
> relatively little gain.
>
> Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org>
> ---
>
> I submitted this as an RFC back in December, and received no comments,
> so I'm submitting this as an actual patch now.  My key concern was the
> final paragraph above -- embedding using the "count" files for something
> other than counts -- but I've mostly convinced myself that refactoring
> this code to separate that out is unlikely to actually cause significant
> pain.

Actual code change looks fine to me as well, though I see Peff has pointed out we might not even need to make it an option.

← back to recent threads