{"thread":{"id":"58999","subject":"[RFC PATCH] test-lib: allow storing counts with test harnesses","startedAt":"2022-12-24T23:09:15Z","lastAt":"2023-04-01T18:56:24Z","messageCount":5,"participants":["Adam Dinwoodie","Jeff King","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469511","messageId":"20221224225200.1027806-1-adam@dinwoodie.org","threadId":"58999","inReplyTo":null,"subject":"[RFC PATCH] test-lib: allow storing counts with test harnesses","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2022-12-24T22:52:00Z","receivedAt":"2022-12-24T23:09:15Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"Currently, test result files are only stored in test-results/*.counts if\n$HARNESS_ACTIVE is not set. This dates from 8ef1abe550 (test-lib: Don't\nwrite test-results when HARNESS_ACTIVE, 2010-08-11), where the\nassumption was that if someone were using a test harness like prove,\nthat would track results and the count files wouldn't be required.\nHowever, as of 49da404070 (test-lib: show missing prereq summary,\n2021-11-20), those files also store the list of git test prerequisites\nthat were missing during the test run, which isn't something that a\ngeneric test harness like prove can provide.\n\nTo allow folk using test harnesses to access the lists of missing\nprerequisites, add a --counts argument to test-lib that will keep these\ncounts files even if a test harness is in use. This means that a\nsubsequent call of, say, `make -C t aggregate-results` will report\nuseful information.\n\nIt might be preferable to do make a wider-ranging change, including\nstoring the missing prerequisites separately from the count files, so\nthe results can be reported regardless of whether the success/failure\ncounts are wanted, but that would be more disruptive and more work for\nrelatively little gain.\n\nSigned-off-by: Adam Dinwoodie \n---\n\nThe key reason I'm submitting this as an RFC is that last paragraph:\nI've tested the below patch, and it achieves what I'm after (letting me\nboth use prove and audit missing prerequisites), but it further embeds\nthe use of the \"count\" files for recording missing prerequisites, where\nit might be preferable to do something a bit more complex that treats\nthe prerequisite reporting as a separate function, rather than a bolt-on\nto the pass/fail/etc. counts.\n\n t/test-lib.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 6db377f68b..bbd9ee0e34 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -157,6 +157,8 @@ parse_option () {\n \tlocal opt=\"$1\"\n \n \tcase \"$opt\" in\n+\t-c|--c|--co|--cou|--coun|--count|--counts)\n+\t\trecord_counts=t ;;\n \t-d|--d|--de|--deb|--debu|--debug)\n \t\tdebug=t ;;\n \t-i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)\n@@ -1282,7 +1284,7 @@ test_done () {\n \n \tfinalize_test_output\n \n-\tif test -z \"$HARNESS_ACTIVE\"\n+\tif test -z \"$HARNESS_ACTIVE\" || test -n \"$record_counts\"\n \tthen\n \t\tmkdir -p \"$TEST_RESULTS_DIR\"\n \n-- \n2.39.0\n\n"},{"id":"473011","messageId":"20230304212220.qkzc2joco5xj7d4s@lucy.dinwoodie.org","threadId":"58999","inReplyTo":"20221224225200.1027806-1-adam@dinwoodie.org","subject":"[PATCH] test-lib: allow storing counts with test harnesses","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2023-03-04T21:22:20Z","receivedAt":"2023-03-04T22:00:48Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"Currently, test result files are only stored in test-results/*.counts if\n$HARNESS_ACTIVE is not set. This dates from 8ef1abe550 (test-lib: Don't\nwrite test-results when HARNESS_ACTIVE, 2010-08-11), where the\nassumption was that if someone were using a test harness like prove,\nthat would track results and the count files wouldn't be required.\nHowever, as of 49da404070 (test-lib: show missing prereq summary,\n2021-11-20), those files also store the list of git test prerequisites\nthat were missing during the test run, which isn't something that a\ngeneric test harness like prove can provide.\n\nTo allow folk using test harnesses to access the lists of missing\nprerequisites, add a --counts argument to test-lib that will keep these\ncounts files even if a test harness is in use. This means that a\nsubsequent call of, say, `make -C t aggregate-results` will report\nuseful information.\n\nIt might be preferable to do make a wider-ranging change, including\nstoring the missing prerequisites separately from the count files, so\nthe results can be reported regardless of whether the success/failure\ncounts are wanted, but that would be more disruptive and more work for\nrelatively little gain.\n\nSigned-off-by: Adam Dinwoodie \n---\n\nI submitted this as an RFC back in December, and received no comments,\nso I'm submitting this as an actual patch now. My key concern was the\nfinal paragraph above -- embedding using the \"count\" files for something\nother than counts -- but I've mostly convinced myself that refactoring\nthis code to separate that out is unlikely to actually cause significant\npain.\n\n t/test-lib.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 6db377f68b..bbd9ee0e34 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -157,6 +157,8 @@ parse_option () {\n \tlocal opt=\"$1\"\n \n \tcase \"$opt\" in\n+\t-c|--c|--co|--cou|--coun|--count|--counts)\n+\t\trecord_counts=t ;;\n \t-d|--d|--de|--deb|--debu|--debug)\n \t\tdebug=t ;;\n \t-i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)\n@@ -1282,7 +1284,7 @@ test_done () {\n \n \tfinalize_test_output\n \n-\tif test -z \"$HARNESS_ACTIVE\"\n+\tif test -z \"$HARNESS_ACTIVE\" || test -n \"$record_counts\"\n \tthen\n \t\tmkdir -p \"$TEST_RESULTS_DIR\"\n \n"},{"id":"473043","messageId":"ZAWq5VFE/UjjtPJS@coredump.intra.peff.net","threadId":"58999","inReplyTo":"20230304212220.qkzc2joco5xj7d4s@lucy.dinwoodie.org","subject":"Re: [PATCH] test-lib: allow storing counts with test harnesses","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-06T08:57:09Z","receivedAt":"2023-03-06T08:57:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 04, 2023 at 09:22:20PM +0000, Adam Dinwoodie wrote:\n\n> Currently, test result files are only stored in test-results/*.counts if\n> $HARNESS_ACTIVE is not set. This dates from 8ef1abe550 (test-lib: Don't\n> write test-results when HARNESS_ACTIVE, 2010-08-11), where the\n> assumption was that if someone were using a test harness like prove,\n> that would track results and the count files wouldn't be required.\n> However, as of 49da404070 (test-lib: show missing prereq summary,\n> 2021-11-20), those files also store the list of git test prerequisites\n> that were missing during the test run, which isn't something that a\n> generic test harness like prove can provide.\n> \n> To allow folk using test harnesses to access the lists of missing\n> prerequisites, add a --counts argument to test-lib that will keep these\n> counts files even if a test harness is in use. This means that a\n> subsequent call of, say, `make -C t aggregate-results` will report\n> useful information.\n\nYour goal seems reasonable. I have to wonder if it is even worth\nrequiring \"--counts\" here, though. Even 8ef1abe550 claims that the I/O\nfrom writing the results files is minimal. And certainly I run under\nprove with \"--root=/some/ram/disk\", and I haven't noticed any difference\nwith and without my usual \"--verbose-log\", which writes a lot more data\ninto test-results/.\n\nSo would it be worth it to just revert 8ef1abe550, and always store the\nmeta-files? That's one less option to support, and one less surprise\nwhen some other feature is built around them.\n\nOr is there some reason that we really want to have a mode where nothing\nis written into t/? From reading 8ef1abe550 it sounded like this was\nmostly a hygiene / optimization thing, and not some special mode we\ncared about supporting.\n\n> t/test-lib.sh | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n\nThe patch itself looks correct, if we want to go with a --counts option.\n\n-Peff\n"},{"id":"473070","messageId":"xmqqr0u1agq8.fsf@gitster.g","threadId":"58999","inReplyTo":"ZAWq5VFE/UjjtPJS@coredump.intra.peff.net","subject":"Re: [PATCH] test-lib: allow storing counts with test harnesses","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-06T18:15:43Z","receivedAt":"2023-03-06T18:16:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So would it be worth it to just revert 8ef1abe550, and always store the\n> meta-files? That's one less option to support, and one less surprise\n> when some other feature is built around them.\n>\n> Or is there some reason that we really want to have a mode where nothing\n> is written into t/? From reading 8ef1abe550 it sounded like this was\n> mostly a hygiene / optimization thing, and not some special mode we\n> cared about supporting.\n\nInteresting thought. It would simplify our lives to have fewer\nconditionally-moving parts.\n\n>> t/test-lib.sh | 4 +++-\n>> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> The patch itself looks correct, if we want to go with a --counts option.\n\nYes.\n\n"},{"id":"474635","messageId":"CABPp-BGBYUHeYtsyM-gYvr0CsKGAmJ1OKqcmnHiKYy0ps6NrCg@mail.gmail.com","threadId":"58999","inReplyTo":"20230304212220.qkzc2joco5xj7d4s@lucy.dinwoodie.org","subject":"Re: [PATCH] test-lib: allow storing counts with test harnesses","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-04-01T18:56:04Z","receivedAt":"2023-04-01T18:56:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Mar 4, 2023 at 2:16 PM Adam Dinwoodie wrote:\n>\n> Currently, test result files are only stored in test-results/*.counts if\n> $HARNESS_ACTIVE is not set. This dates from 8ef1abe550 (test-lib: Don't\n> write test-results when HARNESS_ACTIVE, 2010-08-11), where the\n> assumption was that if someone were using a test harness like prove,\n> that would track results and the count files wouldn't be required.\n> However, as of 49da404070 (test-lib: show missing prereq summary,\n> 2021-11-20), those files also store the list of git test prerequisites\n> that were missing during the test run, which isn't something that a\n> generic test harness like prove can provide.\n>\n> To allow folk using test harnesses to access the lists of missing\n> prerequisites, add a --counts argument to test-lib that will keep these\n> counts files even if a test harness is in use. This means that a\n> subsequent call of, say, `make -C t aggregate-results` will report\n> useful information.\n>\n> It might be preferable to do make a wider-ranging change, including\n\nReplace \"do make\" with either \"do\" or \"make\"?\n\n> storing the missing prerequisites separately from the count files, so\n> the results can be reported regardless of whether the success/failure\n> counts are wanted, but that would be more disruptive and more work for\n> relatively little gain.\n>\n> Signed-off-by: Adam Dinwoodie \n> ---\n>\n> I submitted this as an RFC back in December, and received no comments,\n> so I'm submitting this as an actual patch now. My key concern was the\n> final paragraph above -- embedding using the \"count\" files for something\n> other than counts -- but I've mostly convinced myself that refactoring\n> this code to separate that out is unlikely to actually cause significant\n> pain.\n\nActual code change looks fine to me as well, though I see Peff has\npointed out we might not even need to make it an option.\n"}]}