Re: [RFC/PATCH] Makefile: add test-all target

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

On Wed, Dec 08 2021, Junio C Hamano wrote:

> We ship contrib/ stuff within our primary source tree but except for
> the completion scripts that are tested from our primary test suite,
> their test suites are not run in the CI.
>
> Teach the main Makefile a "test-extra" target, which goes into each
> package in contrib/ whose Makefile has its own "test" target and
> runs "make test" there.  Add a "test-all" target to make it easy to
> drive both the primary tests and these contrib tests from CI and use
> it.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Junio C Hamano <gitster@pobox.com> writes:
>
>> That is an interesting way to demonstrate how orthogonal the issues
>> are, which in turn means that it is not such a big deal to add back
>> the coverage to the part that goes to contrib/scalar/.  As the actual
>> implementation, it is a bit too icky, though.
>
> So, how about doing it this way?  This is based on 'master' and does
> not cover contrib/scalar, but if we want to go this route, it should
> be trivial to do it on top of a merge of ab/ci-updates and js/scalar
> into 'master'.  Good idea?  Terrible idea?  Not good enough?

With the caveat that I think the greater direction here makes no sense,
i.e. scalar didn't need its own build system etc. in the first place, so
having hack-upon-hack to fix various integration issues is clearly worse
than just having it behave like everything else....

... then yes, adding this to the top-level Makefile makes more sense....

>  Makefile                  | 12 +++++++++++-
>  ci/run-build-and-tests.sh | 10 +++++-----
>  2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git i/Makefile w/Makefile
> index d56c0e4aad..ca14558e3c 100644
> --- i/Makefile
> +++ w/Makefile
> @@ -2878,10 +2878,20 @@ export TEST_NO_MALLOC_CHECK
>  test: all
>  	$(MAKE) -C t/ all
>  
> +# Additional tests from places in contrib/ that are prepared to take
> +# "make -C $there test", but expects that the primary build is done
> +# already.
> +test-extra: all
> +	$(MAKE) -C contrib/diff-highlight test
> +	$(MAKE) -C contrib/mw-to-git test
> +	$(MAKE) -C contrib/subtree test
> +
> +test-all:: test test-extra
> +
>  perf: all
>  	$(MAKE) -C t/perf/ all
>  
> -.PHONY: test perf
> +.PHONY: test test-extra test-all perf
>  
>  .PRECIOUS: $(TEST_OBJS)

Which, if we're nitpicking this would be better, i.e. it allows them to
run in parallel, as they won't be defined by only one rule, and will be
listede individuall in the test-all and test-extra prereqs:

diff --git a/Makefile b/Makefile
index d892dbc6c6e..3f47c9f58ad 100644
--- a/Makefile
+++ b/Makefile
@@ -2878,15 +2878,25 @@ export TEST_NO_MALLOC_CHECK
 test: all
 	$(MAKE) -C t/ all
 
+define TMPL_test-extra
+TEST_EXTRA_TARGETS += test-$(1)
+.PHONY: test-$(1)
+test-$(1): all
+	$$(MAKE) -C $(1) test
+endef
+
 # Additional tests from places in contrib/ that are prepared to take
 # "make -C $there test", but expects that the primary build is done
 # already.
-test-extra: all
-	$(MAKE) -C contrib/diff-highlight test
-	$(MAKE) -C contrib/mw-to-git test
-	$(MAKE) -C contrib/subtree test
+$(eval $(call TMPL_test-extra,contrib/diff-highlight))
+$(eval $(call TMPL_test-extra,contrib/mw-to-git))
+$(eval $(call TMPL_test-extra,contrib/subtree))
+
+.PHONY: test-extra
+test-extra:: all $(TEST_EXTRA_TARGETS)
 
-test-all:: test test-extra
+.PHONY: test-all
+test-all: test $(TEST_EXTRA_TARGETS)
 
 perf: all
 	$(MAKE) -C t/perf/ all

> diff --git i/ci/run-build-and-tests.sh w/ci/run-build-and-tests.sh
> index cc62616d80..9da0f26665 100755
> --- i/ci/run-build-and-tests.sh
> +++ w/ci/run-build-and-tests.sh
> @@ -19,7 +19,7 @@ make
>  case "$jobname" in
>  linux-gcc)
>  	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> -	make test
> +	make test-all
> [...]

But I think we're expanding the scope quite a bit here. The reason we
were talking about testing scalar by default is because it uses
libgit.a, so it's not decoupled at all, whereas the "contrib" programs
are only using the built "git" command.

I think it would probably be good to test these anyway, but it's an
argument beyond that which applies to scalar.

I also share Jeff's general concerns that the other stuff in contrib may
not be all that stable.

But I don't see why we should be pursuing this direction of running
certain tests in CI only, as opposed to just under "make test", that
distinction is something new in js/scalar (before that we run libgit.a
test *modes* in CI, but not a different set of tests).