{"thread":{"id":"57607","subject":"[PATCH] test-lib: have --immediate emit valid TAP on failure","startedAt":"2022-03-23T20:51:42Z","lastAt":"2022-03-28T15:59:42Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason","Johannes Schindelin","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"452064","messageId":"patch-1.1-47b236139e6-20220323T204410Z-avarab@gmail.com","threadId":"57607","inReplyTo":null,"subject":"[PATCH] test-lib: have --immediate emit valid TAP on failure","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-23T20:51:31Z","receivedAt":"2022-03-23T20:51:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"--immediate\" option so that it emits valid TAP on\nfailure. Before this it would omit the required plan at the end,\ne.g. under SANITIZE=leak we'd show a \"No plan found in TAP output\"\nerror from \"prove\":\n\n $ prove t0006-date.sh :: --immediate\n t0006-date.sh .. Dubious, test returned 1 (wstat 256, 0x100)\n Failed 1/22 subtests\n\n Test Summary Report\n -------------------\n t0006-date.sh (Wstat: 256 Tests: 22 Failed: 1)\n Failed test: 22\n Non-zero exit status: 1\n Parse errors: No plan found in TAP output\n Files=1, Tests=22, 0 wallclock secs ( 0.02 usr 0.01 sys + 0.18 cusr 0.06 csys = 0.27 CPU)\n Result: FAIL\n\nNow we'll emit output that doesn't result in TAP parsing failures:\n\n $ prove t0006-date.sh :: --immediate\n t0006-date.sh .. Dubious, test returned 1 (wstat 256, 0x100)\n Failed 1/22 subtests\n\n Test Summary Report\n -------------------\n t0006-date.sh (Wstat: 256 Tests: 22 Failed: 1)\n Failed test: 22\n Non-zero exit status: 1\n Files=1, Tests=22, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.19 cusr 0.05 csys = 0.26 CPU)\n Result: FAIL\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nJunio: When this is merged to \"seen\" there's a conflict like:\n\n\t<<<<<<< HEAD\n\t\tif test -n \"$immediate\"\n\t\tthen\n\t\t\tsay_color error \"1..$test_count\"\n\t\t\t_error_exit\n\t\tfi\n\t=======\n\t\ttest \"$immediate\" = \"\" || _error_exit\n\t\tfinalize_test_case_output failure \"$failure_label\" \"$@\"\n\t>>>>>>> origin/seen\n\nThe \"solution\" is to end up with:\n\n\ttest_failure_ () {\n\t\tfailure_label=$1\n\t\ttest_failure=$(($test_failure + 1))\n\t\tsay_color error \"not ok $test_count - $1\"\n\t\tshift\n\t\tprintf '%s\\n' \"$*\" | sed -e 's/^/#\t/'\n\t\tif test -n \"$immediate\"\n\t\tthen\n\t\t\tsay_color error \"1..$test_count\"\n\t\t\t_error_exit\n\t\tfi\n\t\tfinalize_test_case_output failure \"$failure_label\" \"$@\"\n\t}\n\nThose scare quotes being because the \"finalize_test_case_output\" is in\nthe wrong place both before and after, but that's an existing bug in\neb53a5b8047 (ci: call `finalize_test_case_output` a little later,\n2022-03-01), i.e. that alternate output mechanism wasn't doing the\nright thing on --immediate output.\n\nBut this change and that merge resolution doesn't make that any worse,\nso hopefully this can be queued anyway. Thanks!\n\t\t\n t/t0000-basic.sh | 13 +++++++++++++\n t/test-lib.sh | 6 +++++-\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 9dcbf518a78..17a268ccd1b 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -101,6 +101,19 @@ test_expect_success 'subtest: 2/3 tests passing' '\n \tEOF\n '\n \n+test_expect_success 'subtest: --immediate' '\n+\trun_sub_test_lib_test_err partial-pass \\\n+\t\t--immediate &&\n+\tcheck_sub_test_lib_test_err partial-pass \\\n+\t\t<<-\\EOF_OUT 3<<-EOF_ERR\n+\t> ok 1 - passing test #1\n+\t> not ok 2 - failing test #2\n+\t> #\tfalse\n+\t> 1..2\n+\tEOF_OUT\n+\tEOF_ERR\n+'\n+\n test_expect_success 'subtest: a failing TODO test' '\n \twrite_and_run_sub_test_lib_test failing-todo <<-\\EOF &&\n \ttest_expect_success \"passing test\" \"true\"\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 515b1af7ed4..4373f7d70b5 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -806,7 +806,11 @@ test_failure_ () {\n \tsay_color error \"not ok $test_count - $1\"\n \tshift\n \tprintf '%s\\n' \"$*\" | sed -e 's/^/#\t/'\n-\ttest \"$immediate\" = \"\" || _error_exit\n+\tif test -n \"$immediate\"\n+\tthen\n+\t\tsay_color error \"1..$test_count\"\n+\t\t_error_exit\n+\tfi\n }\n \n test_known_broken_ok_ () {\n-- \n2.35.1.1452.ga7cfc89151f\n\n"},{"id":"452114","messageId":"nycvar.QRO.7.76.6.2203241434360.388@tvgsbejvaqbjf.bet","threadId":"57607","inReplyTo":"patch-1.1-47b236139e6-20220323T204410Z-avarab@gmail.com","subject":"Re: [PATCH] test-lib: have --immediate emit valid TAP on failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-03-24T13:38:58Z","receivedAt":"2022-03-24T13:39:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Wed, 23 Mar 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the \"--immediate\" option so that it emits valid TAP on\n> failure. Before this it would omit the required plan at the end,\n> e.g. under SANITIZE=leak we'd show a \"No plan found in TAP output\"\n> error from \"prove\":\n>\n> $ prove t0006-date.sh :: --immediate\n> t0006-date.sh .. Dubious, test returned 1 (wstat 256, 0x100)\n> Failed 1/22 subtests\n>\n> Test Summary Report\n> -------------------\n> t0006-date.sh (Wstat: 256 Tests: 22 Failed: 1)\n> Failed test: 22\n> Non-zero exit status: 1\n> Parse errors: No plan found in TAP output\n> Files=1, Tests=22, 0 wallclock secs ( 0.02 usr 0.01 sys + 0.18 cusr 0.06 csys = 0.27 CPU)\n> Result: FAIL\n>\n> Now we'll emit output that doesn't result in TAP parsing failures:\n>\n> $ prove t0006-date.sh :: --immediate\n> t0006-date.sh .. Dubious, test returned 1 (wstat 256, 0x100)\n> Failed 1/22 subtests\n>\n> Test Summary Report\n> -------------------\n> t0006-date.sh (Wstat: 256 Tests: 22 Failed: 1)\n> Failed test: 22\n> Non-zero exit status: 1\n> Files=1, Tests=22, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.19 cusr 0.05 csys = 0.26 CPU)\n> Result: FAIL\n\nThe commit message is strong on the what, very strong in giving verbose\noutput that might or might not clarify the intention, and a little weak in\nthe why and the greater context.\n\nIn the broader story arc, I wonder why we're even bothering with TAP\nanymore because it seems that the world has moved on to more powerful file\nformats to represent test output, such as JUnit XML, that can be parsed,\nrendered and analyzed by powerful special-purpose applications.\n\nApart from taking reviewer time away from such more powerful avenues to\nmake sense of our failing tests, the diff does not really hurt, so I have\nno objections against integrating this patch.\n\nCiao,\nJohannes\n\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 9dcbf518a78..17a268ccd1b 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -101,6 +101,19 @@ test_expect_success 'subtest: 2/3 tests passing' '\n> \tEOF\n> '\n>\n> +test_expect_success 'subtest: --immediate' '\n> +\trun_sub_test_lib_test_err partial-pass \\\n> +\t\t--immediate &&\n> +\tcheck_sub_test_lib_test_err partial-pass \\\n> +\t\t<<-\\EOF_OUT 3<<-EOF_ERR\n> +\t> ok 1 - passing test #1\n> +\t> not ok 2 - failing test #2\n> +\t> #\tfalse\n> +\t> 1..2\n> +\tEOF_OUT\n> +\tEOF_ERR\n> +'\n> +\n> test_expect_success 'subtest: a failing TODO test' '\n> \twrite_and_run_sub_test_lib_test failing-todo <<-\\EOF &&\n> \ttest_expect_success \"passing test\" \"true\"\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 515b1af7ed4..4373f7d70b5 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -806,7 +806,11 @@ test_failure_ () {\n> \tsay_color error \"not ok $test_count - $1\"\n> \tshift\n> \tprintf '%s\\n' \"$*\" | sed -e 's/^/#\t/'\n> -\ttest \"$immediate\" = \"\" || _error_exit\n> +\tif test -n \"$immediate\"\n> +\tthen\n> +\t\tsay_color error \"1..$test_count\"\n> +\t\t_error_exit\n> +\tfi\n> }\n>\n> test_known_broken_ok_ () {\n> --\n> 2.35.1.1452.ga7cfc89151f\n>\n>\n"},{"id":"452123","messageId":"220324.8635j7nyvw.gmgdl@evledraar.gmail.com","threadId":"57607","inReplyTo":"nycvar.QRO.7.76.6.2203241434360.388@tvgsbejvaqbjf.bet","subject":"A \"why TAP?\" manifesto (was: [PATCH] test-lib: have --immediate emit valid TAP on failure)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-24T13:48:42Z","receivedAt":"2022-03-24T15:39:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 24 2022, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Wed, 23 Mar 2022, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Change the \"--immediate\" option so that it emits valid TAP on\n>> failure. Before this it would omit the required plan at the end,\n>> e.g. under SANITIZE=leak we'd show a \"No plan found in TAP output\"\n>> error from \"prove\":\n>>\n>> $ prove t0006-date.sh :: --immediate\n>> t0006-date.sh .. Dubious, test returned 1 (wstat 256, 0x100)\n>> Failed 1/22 subtests\n>>\n>> Test Summary Report\n>> -------------------\n>> t0006-date.sh (Wstat: 256 Tests: 22 Failed: 1)\n>> Failed test: 22\n>> Non-zero exit status: 1\n>> Parse errors: No plan found in TAP output\n>> Files=1, Tests=22, 0 wallclock secs ( 0.02 usr 0.01 sys + 0.18 cusr 0.06 csys = 0.27 CPU)\n>> Result: FAIL\n>>\n>> Now we'll emit output that doesn't result in TAP parsing failures:\n>>\n>> $ prove t0006-date.sh :: --immediate\n>> t0006-date.sh .. Dubious, test returned 1 (wstat 256, 0x100)\n>> Failed 1/22 subtests\n>>\n>> Test Summary Report\n>> -------------------\n>> t0006-date.sh (Wstat: 256 Tests: 22 Failed: 1)\n>> Failed test: 22\n>> Non-zero exit status: 1\n>> Files=1, Tests=22, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.19 cusr 0.05 csys = 0.26 CPU)\n>> Result: FAIL\n>\n> The commit message is strong on the what, very strong in giving verbose\n> output that might or might not clarify the intention, and a little weak in\n> the why and the greater context.\n\nI thought \"so that it emits valid TAP\" was sufficiently\nself-explaining. I.e. we emit this machine-readable format, but in this\nedge case our output is invalid TAP, now it's valid.\n\nRe the verbose output: The last time I submitted patch for tweaking the\nTAP output I got the feedback (from Junio in\nhttps://lore.kernel.org/git/xmqq7dad10oy.fsf@gitster.g/) that a more\nexplicit example was wanted. So I included it this time around.\n\n> In the broader story arc, I wonder why we're even bothering with TAP\n> anymore because it seems that the world has moved on to more powerful file\n> formats to represent test output, such as JUnit XML, that can be parsed,\n> rendered and analyzed by powerful special-purpose applications.\n\n[I guess that warrants a $subject change]\n\nA few things, grouped by heading:\n\n== It \"can be parsed\"\n\nFirst, the \"[TAP ]can['t] be parsed\" part of that is wrong.\n\nAs your own CI patch series[1] shows we currently parse the entire TAP\noutput of our test suite on every CI run. Otherwise prove(1) couldn't\nhave come up with this (quoting from your [1]):\n\t\n\tTest Summary Report\n\t-------------------\n\tt1092-sparse-checkout-compatibility.sh (Wstat: 256 Tests: 53 Failed: 1)\n\t Failed test: 49\n\t Non-zero exit status: 1\n\tt3701-add-interactive.sh (Wstat: 0 Tests: 71 Failed: 0)\n\t TODO passed: 45, 47\n\nThat's the result of a compliant TAP parser parsing all of our test\noutput, which has worked for all our tests (sans edge cases like the one\nbeing fixed here) for a long time.\n\nLet's not go into how you don't like that summary output, that's not the\npoint.\n\nThat's a UX layer presenting something from the fully parsed output, the\npoint is that it *is* not only parsable, but it's currently *being\nparsed* by our toolchain on every CI run.\n\n== Human readable\n\nUnless you're expecting us to eyeball something like this verbose XML on\nad-hoc runs like:\n\t\n\t$ ./t0013-sha1dc.sh --write-junit-xml --tee\n\tok 1 - test-sha1 detects shattered pdf\n\t# passed all 1 test(s)\n\t1..1\n\nWhich for JUnit is:\n\t\n\t$ cat out/TEST-t0013-sha1dc.xml \n\t\n\t \n\t \n\t \n\t \n\t\n\nYou're going to end up with 2 output formats, a human readable one and\nsome secondary also-machine-readable output.\n\nTAP isn't perfect, but it's uniquely suitable as a format for the sort\nof testing we do, because it's explicitly machine-readable format that's\ndesigned to also be human readable.\n\nWhich isn't a goal with JUnit. Perhaps some XML enthusiasts would argue\nthat point. Sure, you *can* read it, but it's not exactly pleasing nor\naiming to reduce verbosity.\n\nThe JUnit we currently emit is >2x the verbosity of TAP by lines, and\n~3x as much by byte count (all of which a human needs to occasionally\nskim):\n\t\n\t$ rm -rf out; ./t0001-init.sh --write-junit-xml --tee |wc ; wc &1 | tail -n 1\n\t100.00 3.703689 21 171581 13587 total\n\t$ strace --summary-only -f ./t0001-init.sh --tee --write-junit-xml 2>&1 | tail -n 1\n\t100.00 6.424080 22 285903 21097 total\n\nI.e. the implementation of it just needs an \"echo\" or whatever, whereas\nthe XML one is shelling out for pretty much every line it prints to\nescape things.\n\nI'm you might argue about the hatefulness about shellscripting there and\nhow we should rewrite everything in C.\n\nI don't disagree with that point in the abstract, but realistically\nwe'll be needing to deal with the shellscript-based test suite for the\nforeseeable future.\n\n== It's more flexible\n\nThe JUnit we write out is just the summary output, but when you run a\ntest with e.g. \"--verbose -x\" the result is valid TAP. I.e. TAP parser\nwill take this:\n\n\t$ ./t0013-sha1dc.sh -vx 2>&1|wc\n\t38 165 1104\n\nAnd grab the lines matching \"ok\" \"not ok\" etc., and *ignore everything\nelse*.\n\nThis means that we're able to use it for things that you just couldn't\nuse JUnit for, e.g. the POC I have here where we pair up our \"--verbose\n-x\" output with the failure summary, i.e. show what specific test things\nbelongs t [2]:\n\n== It doesn't matter if nobody else uses it\n\nOr: \"we don't need a 2nd machine-parsable format\".\n\nI think your \"the world has moved on to more powerful file formats to\nrepresent test output\" is a valid concern.\n\nI.e. we don't want to have a test suite output that we can't pretty\nrender on whatever the latest fancy test CI renderer is is a completely\nvalid concern, whether that's Azure or GitHub CI.\n\nBut the implicit assumption there seems to be that TAP isn't rather\ntrivially machine-parsable, and that we aren't already running a parser\nfor it. It is, and we are.\n\nBecause of that we can just have a post-transformation step of changing\nTAP into whatever other format you'd like.\n\nSo you can have JUnit, GitHub CI Markdown or whatever, it should just be\nproduced from the TAP output.\n\nI think that your addition of JUnit & GitHub CI Markdown (in [1]) is\nmuch better done like that. It avoids the complexity of N output formats\nin test-lib.sh. A transformer for your desired format probably exists\nalready (or could trivially be adjusted for a new format):\n\n https://metacpan.org/search?q=TAP%3A%3AFormatter\n\n== Everyone uses it\n\nWell, , but while per the above I don't think wide use\nof TAP even matters I think being concerned about lack of tooling\nsupport for it is strange.\n\nI'm fairly sure it's the single most widely supported test output format\nof any kind. Because of how trivial it is to parse and implement I'm\npretty sure it's the most widely-supported machine-readable test output\nformat of any kind.\n\n*Maybe* something like JUnit has (near) parity if we're counting format\ntransformers-to-transformers, but I'm fairly sure not if it wouldn't be\nallowed to \"cheat\" by making use of TAP as an intermediate format.\n\n== Cases where TAP isn't good enough currently\n\nSomeone familiar with our test tooling might thing \"wait, TAP with\n--verbose is valid TAP?\" because since 614fe015212 (test-lib: bail out\nwhen \"-v\" used under \"prove\", 2016-10-22) we have been refusing to\nproduce it in that case:\n\n $ prove t0013-sha1dc.sh :: --verbose\n Bailout called. Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log\n FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log\n\nFixing that is needed for the approach in [2] to work 100% of the time,\nI have working patches to that (which as it happens, are currently\nblocking on conflicts with your [1]).\n\nI.e. the problem is that if we're expecting a line starting with \"ok\" or\nwhatever the firehose --verbose output from the test itself might emit\nsuch a line, and confuse the parser.\n\nThe solution was rather trivial in the case of TAP, just have\ntest-lib.sh emit a configurable prefix string before any TAP syntax it\nemits, then have a \"tee\" tool we ship as a test helper munge that after\nthe fact:\n\n https://github.com/avar/git/commit/2358ff8c3a7\n https://github.com/avar/git/commit/2ae430bfce7\n\n== Handling the long-tail of machine-readability\n\nContinuing the above: I don't think a solution to that sort of thing is\nreally viable with anything that doesn't behave like TAP.\n\nWe have tests doing things like starting a background daemon that will\nracily emit output its output *in the middle of another* test.\n\nAs in not theoretically, it's a case I actually had to debug & test to\nget the above commits working. The JUnit emitter punts on it entirely by\nnot including the \"--verbose -x\" output at all.\n\nThe rather easy solution for TAP explicitly relies on its \"non-syntax\"\nof considering every line it doesn't explicitly classify as syntax as\nsomething it'll pass through as \"output\".\n\nThose lines thenh need no further escaping. They can pretty much be\nlatin-1, UTF-8, your favorite bits from /dev/urandom etc.. It doesn't\nmatter.\n\nWhereas for e.g. any XML, JSON etc. format where things are syntax by\ndefault you'd need to intercept all that output to strip \"<\" and the\nlike from it.\n\nEven if you did that you probably wouldn't have anywhere sensible to\nstick that output, you probably already emitted the beginning/end\nmarkers for \"here's the verbose output for test N\" already, which would\nrequire complexity a TAP emitter wouldn't need.\n\n> Apart from taking reviewer time away from such more powerful avenues to\n> make sense of our failing tests, the diff does not really hurt, so I have\n> no objections against integrating this patch.\n\nI hope the above will summarize why those suggested avenues aren't \"more\npowerful\", quite the opposite.\n\nI think if you did the legwork of trying to make those formats represent\nour \"--verbose -x\" output in a machine-readable way and tried to\nroundtrip that parsed output (which I have done for TAP) that you'd find\nit somewhere between \"much, much harder\" and \"impossible\" to do the same\nfor those other formats in the context of our test suite.\n\nSo yes, I agree that there's a lack of focus here, and we should put\nmore wood behind fewer arrows in terms of making our test output\nmachine-parsable.\n\nThe [2] I mentioned above shows the sorts of wins we can get from that,\ni.e. emitting *only* the lines of test output relevant to the specific\nfailures we had.\n\nThat's really useful, and something you inherently can't do with the\nsort of approach you're going for in your [1] series.\n\nAt least not without buffering the whole thing & parsing it, at which\npoint you're back to square #1 in terms of fixing the sorts of issues\nnoted in \"Handling the long-tail of machine-readability\" above.\n\n1. https://lore.kernel.org/git/pull.1117.git.1643050574.gitgitgadget@gmail.com/\n2. https://lore.kernel.org/git/220302.86mti87cj2.gmgdl@evledraar.gmail.com/\n\n"},{"id":"452197","messageId":"xmqqczibjm7v.fsf@gitster.g","threadId":"57607","inReplyTo":"220324.8635j7nyvw.gmgdl@evledraar.gmail.com","subject":"Re: A \"why TAP?\" manifesto","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-24T17:26:28Z","receivedAt":"2022-03-24T17:26:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> == It \"can be parsed\"\n> == Human readable\n> == It's trivial to produce\n> == It's more flexible\n> == It doesn't matter if nobody else uses it\n> == Everyone uses it\n\nI agree with all of what you said above.\n\nBut do non-human readers really want to read output from an\n\"--immediate\" session? Don't they rather see the whole thing?\n\nThe only \"Huh?\" I had with the original patch that started the\nthread was that TAP output currently does not work well with the\n\"--verbose\" option, and I've never run our tests with the\n\"--immediate\" option without the \"--verbose\" option to see where and\nhow the first breakage happens and what is left behind in the trash\ndirectory, i.e. to bootstrap an interactive debugging session.\n\nBut not being useful for the use case of human reader running\ninteractively to get the leftover state does not mean improvement\nfor other use cases is not welcome.\n\n> I think if you did the legwork of trying to make those formats represent\n> our \"--verbose -x\" output in a machine-readable way and tried to\n> roundtrip that parsed output (which I have done for TAP) that you'd find\n> it somewhere between \"much, much harder\" and \"impossible\" to do the same\n> for those other formats in the context of our test suite.\n>\n> So yes, I agree that there's a lack of focus here, and we should put\n> more wood behind fewer arrows in terms of making our test output\n> machine-parsable.\n>\n> The [2] I mentioned above shows the sorts of wins we can get from that,\n> i.e. emitting *only* the lines of test output relevant to the specific\n> failures we had.\n>\n> That's really useful, and something you inherently can't do with the\n> sort of approach you're going for in your [1] series.\n>\n> At least not without buffering the whole thing & parsing it, at which\n> point you're back to square #1 in terms of fixing the sorts of issues\n> noted in \"Handling the long-tail of machine-readability\" above.\n>\n> 1. https://lore.kernel.org/git/pull.1117.git.1643050574.gitgitgadget@gmail.com/\n> 2. https://lore.kernel.org/git/220302.86mti87cj2.gmgdl@evledraar.gmail.com/\n"},{"id":"452234","messageId":"YjzpYic+QaOpqolE@nand.local","threadId":"57607","inReplyTo":"220324.8635j7nyvw.gmgdl@evledraar.gmail.com","subject":"Re: A \"why TAP?\" manifesto (was: [PATCH] test-lib: have --immediate emit valid TAP on failure)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-03-24T21:57:54Z","receivedAt":"2022-03-24T21:57:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 24, 2022 at 02:48:42PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > The commit message is strong on the what, very strong in giving verbose\n> > output that might or might not clarify the intention, and a little weak in\n> > the why and the greater context.\n>\n> I thought \"so that it emits valid TAP\" was sufficiently\n> self-explaining. I.e. we emit this machine-readable format, but in this\n> edge case our output is invalid TAP, now it's valid.\n\nI agree; if the justification is \"something we use not-infrequently is\nbroken\" and the rest is \"and this patch un-breaks it\", I do not think we\nshould devote much space to justifying why we use that thing in the\nfirst place.\n\nOur TAP output meets the bar (at least for me, personally) of not\nneeding to be rehashed anytime we change it, so I don't have any\ncomplaints about Ævar's patch message here.\n\nOf course, we should be careful to avoid following that guidance _too_\nmuch, since if it leaves us in a spot where we never question any past\ndecisions, then I think we have gone too far.\n\nThanks,\nTaylor\n"},{"id":"452481","messageId":"220328.8635j23w61.gmgdl@evledraar.gmail.com","threadId":"57607","inReplyTo":"xmqqczibjm7v.fsf@gitster.g","subject":"Re: A \"why TAP?\" manifesto","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-28T15:50:39Z","receivedAt":"2022-03-28T15:59:42Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 24 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> == It \"can be parsed\"\n>> == Human readable\n>> == It's trivial to produce\n>> == It's more flexible\n>> == It doesn't matter if nobody else uses it\n>> == Everyone uses it\n>\n> I agree with all of what you said above.\n>\n> But do non-human readers really want to read output from an\n> \"--immediate\" session? Don't they rather see the whole thing?\n\nThat test run will still take longer, especially if you're expecting\nwidespread failures.\n\nThe immediate use-case I have for this is that I have unsubmitted\npatches to implement a \"check\" mode for SANITIZE=leak, where it runs\nwith --immediate and \"fails\" tests that pass under SANITIZE=leak but\naren't marked with \"TEST_PASSES_SANITIZE_LEAK=true\", and passes (and\nruns to completion completely) those tests that do have\n\"TEST_PASSES_SANITIZE_LEAK=true\" but pass with SANITIZE=leak (and fails\nthose that would fail there, no \"fail inversion\").\n\nThat sort of thing is very useful to for any tests where we mark certain\nother tests as being OK under a given mode, and except there to be a 1=1\ncorrespondance.\n\nThe reason for the --immediate there is an optimization, it takes a long\ntime to run with SANITIZE=leak, and if any single failure is enough to\ndeclare the test bad --immediate is useful.\n\nNow, the TAP tooling is quite tolerant of such bad output, another point\nin its favor over something like JUnit (I'm assuming most libraries\nusing it would pass it through a generic XML parser, whose error\nhandling is either \"ok\" or \"ARGHL!\" :).\n\nI.e. it'll give you a full parse, but just have a minor complaint about\nthe lacking end marker, getting rid of that output & parse nit is nice.\n\n> The only \"Huh?\" I had with the original patch that started the\n> thread was that TAP output currently does not work well with the\n> \"--verbose\" option, and I've never run our tests with the\n> \"--immediate\" option without the \"--verbose\" option to see where and\n> how the first breakage happens and what is left behind in the trash\n> directory, i.e. to bootstrap an interactive debugging session.\n>\n> But not being useful for the use case of human reader running\n> interactively to get the leftover state does not mean improvement\n> for other use cases is not welcome.\n\nI was sad to see that ability go away, without the patches I referenced\nrunning through a TAP parser with --verbose works for almost all ouf our\ntest suite, so IMO we were a bit overzelous in adding that early abort\nfor it.\n\nBut I do have patches to make it pass 100% guaranteed, so depending on\nyour interest ... :)\n\nI was planning to queue that behind some of the outstanding test lib\nchanges it would conflict with, e.g. this one & the outstanding CI\ntopics.\n"}]}