Re: [PATCH v10 00/15] Upstreaming the Scalar command

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

On Sun, Dec 05 2021, Junio C Hamano wrote:

> Elijah Newren <newren@gmail.com> writes:
>
>> From your wording it sounds like the plan might not include moving
>> these tests over.  Perhaps it doesn't make sense to move them all
>> over, but since they've caught problems in both Scalar and core Git,
>> it would be nice to see many of those tests come to Git as well as
>> part of a future follow on series.
>
> Yeah, we may be initially queuing this without tests for expediency,
> but a production code cannot go forever without CI tests to ensure
> continued code health.  People make changes in other parts of the
> system Scalar may depend on and unknowingly break some assumption
> Scalar makes on it.

In this case there really isn't any reason not to have the tests go in
at the same time. The explanation in the v10 CL is:
    
    Changes since v9:
    
     * The patches to build Scalar and run its tests as part of Git's CI/PR,
       have been dropped because a recent unrelated patch series does not
       interact well with them.

That assessment isn't correct.

The change in v8->v9 of adding a "make &&" before the "test" was only
necessary because of a logic error in the v8 version. Yes it broke
because the "scalar test" target didn't know how to build its
prerequisites, but the real underlying issue is that it was even trying
at that point. It had no business running in the static-analysis target
where we hadn't built git already.

Now v9->v10 has
dropped the tests entirely, allegedly due to an interaction with my
ab/ci-updates, but there's nothing new there that isn't also the case on
"master".

But we can have our cake and eat it too.

The below patch on top of v9 would make the scalar tests do the right
thing. I.e. whenever we do a "make test" we'll run the scalar tests
too.

The code changes somewhat with ab/ci-updates, but the conflict with
js/scalar is mostly textual, not semantic (and as I've pointed out, to
the extent that ab/ci-updates changed anything it made things a bit
better for js/scalar).

I'd really like to see this scalar series land, but I really don't see
why it's necessary to entirety eject the CI test coverage due to what's
a rather trivilly solved issue.

As I've noted ad-nauseum at this point I think the necessity for the
below patch is rather silly, this should just nicely integrate with
"make test", but <brokenrecord.gif>. But even without that IMO better
approach it's clearly rather trivial to make this series have test
coverage.

It was just broken because it added a test run to the "pedantic" run,
and didn't properly integrate with the multi-"make test" runs on
"master" , both of which are addressed by the patch below.

diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 2ef9fbfdd38..af99699f82b 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -15,6 +15,26 @@ then
 	export DEVOPTS=pedantic
 fi
 
+make() {
+	scalar_tests=
+	for target
+	do
+		if test $target = "test"
+		then
+			scalar_tests=t
+		fi
+	done
+
+	# Do whatever we would have done with "make"
+	command make "$@"
+
+	# Running tests? Run scalar tests too
+	if test -n "$scalar_tests"
+	then
+		command make -C contrib/scalar test
+	fi
+}
+
 make
 case "$jobname" in
 linux-gcc)
@@ -52,6 +72,4 @@ esac
 
 check_unignored_build_artifacts
 
-make && make -C contrib/scalar test
-
 save_good_tree