never refactor v.s. testing (was: [PATCH] tests: fix incorrect --write-junit-xml code)

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jul 14 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [...]
>     As if another data point was needed to corroborate my claim that
>     refactorings are neither free of cost nor of risk, this patch fixes a
>     regression I myself introduced while refactoring the JUnit XML output
>     code. At least this refactoring was motivated by an ulterior goal to
>     improve Git's contributor experience, not just refactoring for
>     refactoring's sake.
>     
>     Unfortunately, I noticed this regression no earlier than when I needed
>     to validate Git for Windows v2.37.1. Since v2.37.1 was an embargoed
>     release, I could not use GitHub Actions for the CI testing, so I had to
>     reinstate Git's Azure Pipeline.
>     
>     It will probably surprise only few, if at all, that this is far from the
>     only regression in the CI code that I had to fix just so I could run the
>     Azure Pipeline successfully. I plan on contributing all of these
>     regression fixes, of course, packaged into neat little,
>     logically-separate patch series that should be easy on reviewers.

I take your point that any code change carries risk, but I really think
walking away with the lesson that "refactorings" are bad is the wrong
thing to draw from this.

This is a textbook example of the value of testing. Code can break
because it's refactored directly, or because a library or tool it
depended on changed, etc.

The problem here isn't that we changed some code, but that it was
changed blindly.

Hindsight is 20/20 on the specifics of any bug, and I don't mean to pick
on you for having made a change without adding tests here in
particular. I've also done it, and will probably do it in the future
where I'm overly sure of myself.

But the idea that we should first consider basic lack of test coverage
before making (further) changes really isn't 20/20 hindsight. I think it
should be a basic part of our workflow, particularly in cases like this
(and the bisect case I pointed out in another thread[1]) where we can
see that we could remove the entire feature (or sub-feature for [1]) and
still pass 100% of our test.

As test coverage approaches 100% (which is often impossible in practice)
the risk of refactorings approaches (but never reaches) 0%. Plot them on
a mental graph and they're roughly inversely proportional to one
another. We'll almost never reach 100% and 0% in practice.

In this case we have no test coverage for --write-junit-xml before or
after this change. Before it were dying right away with just running
test-lib.sh with --write-junit-xml, So even the below up to
"test_must_be_empty" would catch it.

So we don't need the hypothetical 100%, just getting to 0.1% is enough,
and the below hopefully gets us past that to 1% (a lot of
--write-junit-xml remains completely untested):

1. https://lore.kernel.org/git/220714.86fsj4356l.gmgdl@evledraar.gmail.com/

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 17a268ccd1b..f3c12314787 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -238,6 +238,34 @@ test_expect_success 'subtest: --verbose option' '
 	EOF
 '
 
+test_expect_success 'subtest: --write-junit-xml option' '
+	# both --write-junit-xml and lib-subtest.sh expect to use "out"
+	GIT_TEST_JUNIT_DIRECTORY=.junit-xml &&
+	export GIT_TEST_JUNIT_DIRECTORY &&
+
+	write_and_run_sub_test_lib_test_err write-junit-xml \
+		--write-junit-xml <<-\EOF &&
+	test_expect_success "--true <>" "true"
+	test_expect_success "--false <>" "false"
+	test_done
+	EOF
+	test_must_be_empty write-junit-xml/err &&
+
+	sed -e "s/^> //" -e "s/time[^>]*/.../g" >expect <<-\EOF &&
+	> <testsuites>
+	>   <testsuite name="write-junit-xml" ...>
+	>     <testcase name="write.1 --true &lt;&gt;" classname="write" ...>
+	>     </testcase>
+	>     <testcase name="write.2 --false &lt;&gt;" classname="write" ...>
+	>       <failure message="not ok 2 - --false &lt;&gt;"> false&#x0a;</failure>
+	>     </testcase>
+	>   </testsuite>
+	> </testsuites>
+	EOF
+	sed -e "s/time[^>]*/.../g" <write-junit-xml/.junit-xml/TEST-write-junit-xml.xml >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'subtest: --verbose-only option' '
 	run_sub_test_lib_test_err \
 		t1234-verbose \
diff --git a/t/test-lib-junit.sh b/t/test-lib-junit.sh
index 79c31c788b9..2aac1c6c548 100644
--- a/t/test-lib-junit.sh
+++ b/t/test-lib-junit.sh
@@ -22,7 +22,7 @@
 # that are are called at the appropriate times during the test runs.
 
 start_test_output () {
-	junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
+	junit_xml_dir="$TEST_OUTPUT_DIRECTORY/${GIT_TEST_JUNIT_DIRECTORY:-out}"
 	mkdir -p "$junit_xml_dir"
 	junit_xml_base=${1##*/}
 	junit_xml_path="$junit_xml_dir/TEST-${junit_xml_base%.sh}.xml"