{"thread":{"id":"52787","subject":"[PATCH] tests: fix --write-junit-xml with subshells","startedAt":"2020-02-12T11:28:00Z","lastAt":"2020-02-12T17:19:24Z","messageCount":2,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"391599","messageId":"pull.552.git.1581506876000.gitgitgadget@gmail.com","threadId":"52787","inReplyTo":null,"subject":"[PATCH] tests: fix --write-junit-xml with subshells","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-12T11:27:55Z","receivedAt":"2020-02-12T11:28:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn t0000, more precisely in its `test_bool_env` test case, there are two\nsubshells that are supposed to fail. To be even _more_ precise, they\nfail by calling the `error` function, and that is okay, because it is in\na subshell, and it is expected that those two subshell invocations fail.\n\nHowever, the `error` function also tries to finalize the JUnit XML (if\nthat XML was asked for, via `--write-junit-xml`. As a consequence, the\nXML is edited to add a `time` attribute for the `testsuite` tag. And\nsince there are two expected `error` calls in addition to the final\n`test_done`, the `finalize_junit_xml` function is called three times and\nnaturally the `time` attribute is added _three times_.\n\nAzure Pipelines is not happy with that, complaining thusly:\n\n ##[warning]Failed to read D:\\a\\1\\s\\t\\out\\TEST-t0000-basic.xml. Error : 'time' is a duplicate attribute name. Line 2, position 82..\n\nOne possible way to address this would be to unset `write_junit_xml` in\nthe `test_bool_env` test case.\n\nBut that would be fragile, as other `error` calls in subshells could be\nintroduced.\n\nSo let's just modify `finalize_junit_xml` to remove any `time` attribute\nbefore adding the authoritative one.\n\nSigned-off-by: Johannes Schindelin \n---\n Fix a warning in the Azure Pipelines runs\n \n Yet another thing I stumbled across...\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-552%2Fdscho%2Fwrite-junit-xml-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-552/dscho/write-junit-xml-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/552\n\n t/test-lib.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 44df51be8f..0ea1e5a05e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1083,7 +1083,8 @@ finalize_junit_xml () {\n \n \t\t# adjust the overall time\n \t\tjunit_time=$(test-tool date getnanos $junit_suite_start)\n-\t\tsed \"s/]*/& time=\\\"$junit_time\\\"/\" \\\n+\t\tsed -e \"s/\\(]*/& time=\\\"$junit_time\\\"/\" \\\n \t\t\t<\"$junit_xml_path\" >\"$junit_xml_path.new\"\n \t\tmv \"$junit_xml_path.new\" \"$junit_xml_path\"\n \n\nbase-commit: de93cc14ab7e8db7645d8dbe4fd2603f76d5851f\n-- \ngitgitgadget\n"},{"id":"391604","messageId":"xmqqftffvi3t.fsf@gitster-ct.c.googlers.com","threadId":"52787","inReplyTo":"pull.552.git.1581506876000.gitgitgadget@gmail.com","subject":"Re: [PATCH] tests: fix --write-junit-xml with subshells","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-12T17:19:18Z","receivedAt":"2020-02-12T17:19:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In t0000, more precisely in its `test_bool_env` test case, there are two\n> subshells that are supposed to fail. To be even _more_ precise, they\n> fail by calling the `error` function, and that is okay, because it is in\n> a subshell, and it is expected that those two subshell invocations fail.\n>\n> However, the `error` function also tries to finalize the JUnit XML (if\n> that XML was asked for, via `--write-junit-xml`. As a consequence, the\n> XML is edited to add a `time` attribute for the `testsuite` tag. And\n> since there are two expected `error` calls in addition to the final\n> `test_done`, the `finalize_junit_xml` function is called three times and\n> naturally the `time` attribute is added _three times_.\n>\n> Azure Pipelines is not happy with that, complaining thusly:\n>\n> ##[warning]Failed to read D:\\a\\1\\s\\t\\out\\TEST-t0000-basic.xml. Error : 'time' is a duplicate attribute name. Line 2, position 82..\n>\n> One possible way to address this would be to unset `write_junit_xml` in\n> the `test_bool_env` test case.\n>\n> But that would be fragile, as other `error` calls in subshells could be\n> introduced.\n\nInteresting. Naïvely, I would have expected that the right fix,\nespecially after reading \"as other error calls can be introduced\"\nwas to stop calling finalize from error() and instead call it from\nthe exit trap, but that would not work, as the exit trap would also\nbe called even from a subshell X-<.\n\n> So let's just modify `finalize_junit_xml` to remove any `time` attribute\n> before adding the authoritative one.\n\nIt feels wrong to repeatedly rewrite the $junit_xml_path file during\nthe test, and it feels doubly wrong to call finalize many number of\ntimes, but given that we do not have a good way to revamp the code\nto stop doing so, I think \"strip existing time attribute, if any,\nand then add what we have\" we see here is the best solution\navailable.\n\nWill queue. Thanks.\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 44df51be8f..0ea1e5a05e 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1083,7 +1083,8 @@ finalize_junit_xml () {\n> \n> \t\t# adjust the overall time\n> \t\tjunit_time=$(test-tool date getnanos $junit_suite_start)\n> -\t\tsed \"s/]*/& time=\\\"$junit_time\\\"/\" \\\n> +\t\tsed -e \"s/\\( +\t\t\t-e \"s/]*/& time=\\\"$junit_time\\\"/\" \\\n> \t\t\t<\"$junit_xml_path\" >\"$junit_xml_path.new\"\n> \t\tmv \"$junit_xml_path.new\" \"$junit_xml_path\"\n> \n>\n> base-commit: de93cc14ab7e8db7645d8dbe4fd2603f76d5851f\n"}]}