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

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 <redacted>
---
Junio C Hamano [off-list ref] 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?

 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)
 

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
 	export GIT_TEST_SPLIT_INDEX=yes
 	export GIT_TEST_MERGE_ALGORITHM=recursive
 	export GIT_TEST_FULL_IN_PACK_ARRAY=true

@@ -33,20 +33,20 @@ linux-gcc)
 	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
 	export GIT_TEST_WRITE_REV_INDEX=1
 	export GIT_TEST_CHECKOUT_WORKERS=2
-	make test
+	make test-all
 	;;
 linux-clang)
 	export GIT_TEST_DEFAULT_HASH=sha1
-	make test
+	make test-all
 	export GIT_TEST_DEFAULT_HASH=sha256
-	make test
+	make test-all
 	;;
 linux-gcc-4.8|pedantic)
 	# Don't run the tests; we only care about whether Git can be
 	# built with GCC 4.8 or with pedantic
 	;;
 *)
-	make test
+	make test-all
 	;;
 esac
 

On 12/8/2021 3:04 PM, 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.

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?

+# 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

I like how this is obviously extendible to include contrib/scalar
in a later change, then remove it when Scalar moves.

+test-all:: test test-extra

And this test-all implies that test runs before test-extra, so
libgit.a is compiled appropriately.

I think this approach looks good to me.

Since we are now building and testing things that we have not been
testing recently, it is worth checking that we don't have any work
to do to make this pass. I assume that you've run 'make test-all'
on your own machine. It will be good to see what the full action
reports (probably all good).

Thanks,
-Stolee

On Wed, Dec 08, 2021 at 12:04:50PM -0800, Junio C Hamano wrote:

quoted

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?

I don't mind the general direction, but...

+# 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

I'm not sure of the quality of tests in some of the contrib stuff. The
tests in diff-highlight worked for me when I added them, but it's not
like I ever run them regularly, or that they've been tested on a wide
variety of platforms.

So I think this is as likely to cause somebody a headache due to a dumb
portability problem or random bitrot as it is to actually find a bug. I
guess test-extra wouldn't be run by default, but only via CI, so maybe
that limits the blast radius sufficiently.

For diff-highlight in particular, you need to have a working perl, so
you'd probably want to at least wrap it with a NO_PERL ifndef. For
mw-to-git, you need to have MediaWiki::API installed, though I think the
tests at least notice this and skip everything if you don't.

-Peff

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 <redacted>
---
Junio C Hamano [off-list ref] writes:

quoted

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....

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

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).

Hi Junio,

On Wed, 8 Dec 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.

That sends a strong message that the stuff in contrib/ is now fully under
your maintenance, i.e. first-class supported.

If I were you, I wouldn't.

Junio C Hamano [off-list ref] writes:

quoted

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/.

I'd rather focus, _some_ focus, on the actual Scalar idea and code.

quoted

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?

Peff mentioned a couple of times how tedious it is to address CI failures
e.g. in the Windows part of Git's CI runs.

So it makes only sense to avoid the same problem with contrib/scalar/
altogether, especially as long as you keep saying that you are still
uncertain whether it will make it into Git as a top-level command.

Which is a strong argument in favor of just leaving the CI part of
contrib/scalar/ out for now, and let it remain _my_ responsibility to
react to any build/test problems arising from unrelated patch series
entering `seen`.

Doing it that way would also have the benefit of allowing more focus on
the actual code in contrib/scalar/scalar.c.

Not that it needs more review, I don't think, as both Stolee and Elijah
gave their thumbs-up already, and I've not received any feedback that
would require further changes to `scalar.c`, at least as of _this_ patch
series.

Ciao,
Dscho

On 09/12/21 03.04, 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 <redacted>

No test failures found with test-all on my system.

Tested-by: Bagas Sanjaya <redacted>

-- 
An old man doll... just what I always wanted! - Clara

Johannes Schindelin [off-list ref] writes:

quoted

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.

That sends a strong message that the stuff in contrib/ is now fully under
your maintenance, i.e. first-class supported.

I do not think running tests on stuff in contrib/ sends any such
message.  It primarily helps _us_ to catch more regressions than we
may otherwise miss.  By the way, this is not limited to contrib/; if
we had tests for gitk, we would have caught the recent regression in
"diff -m" before it got inflicted on the general public, but that
would not have been just to help "gitk", but to help keep "diff -m"
sane and stable [*].

By running tests on in-tree contrib/ like scalar, at least we would
notice when we are making breaking changes.  At least, the need for
scalar (either for the API broken by such a change to be kept
unchanged or done in a different way, or the code that uses the API
on the scalar side to be updated) would be noticed earlier than
stuff totally outside and not even in contrib/.

Of course, you have to bear the burden of (A) changing the way
scalar uses the API, or (B) participating in the design of the
change to the API that may break scalar's use so that everybody
including scalar would be happy, or both.  It's not like I am
responsible for everything that happens in the tree, and it is our
shared responsibility to maintain the health of the codebase.  It is
not limited to stuff inside or outside contrib/.

There are projects that want to use libgit.a by binding us as a
submodule and without interacting with us very much.  And they are
on their own when we change the internals.  Do you mean that you
want to make scalar into the same status as they are?

Not that it needs more review, I don't think, as both Stolee and Elijah
gave their thumbs-up already, and I've not received any feedback that
would require further changes to `scalar.c`, at least as of _this_ patch
series.

So that argues even more to have a way to make sure we catch
unintended breakages by any future mindless tree-wide "clean-ups"
and interface changes, no?


[Footnote]

* I just double checked the candidates for "test-extra" to see if
  they are meant to run with a random Git they happen to see on the
  $PATH, or they are designed to test with the version of Git we
  just built, and it seems it is the latter for the ones nominated
  in the test-extra patch.  Otherwise it would indeed reduce the
  benefit in half---we are not helping to catch regressions in the
  core stuff in such a case.

On Mon, Dec 13, 2021 at 12:42:37AM -0800, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

quoted

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.

That sends a strong message that the stuff in contrib/ is now fully under
your maintenance, i.e. first-class supported.

I do not think running tests on stuff in contrib/ sends any such
message.  It primarily helps _us_ to catch more regressions than we
may otherwise miss.  By the way, this is not limited to contrib/; if
we had tests for gitk, we would have caught the recent regression in
"diff -m" before it got inflicted on the general public, but that
would not have been just to help "gitk", but to help keep "diff -m"
sane and stable [*].

I'd actually be a lot more sympathetic to automatically running gitk
tests, because it's just consuming the public API of git (i.e., the
scriptable plumbing interface). If we accidentally break that, it is the
problem of the person who made the breaking change, and we would want
them to know it as soon as possible.

With something like scalar, though, it is adding new callers of the
private API. It might be useful for somebody doing tree-wide refactoring
to know they've broken something there. But it might also be a hassle,
because now they have to care about fixing it, if they are interested in
un-breaking their build (or un-breaking CI). The scalar code is now
their problem, even though it's "just" in contrib/.

In other words, it comes down to a question of where the burden for
fixing things lies. Of course it is nice if somebody doing tree-wide
refactoring fixes up scalar, too. But by making it optional to build
and/or test stuff in contrib/ (rather than tying it to "make all" or to
CI), it lets people decide how nice they want to be.

For other stuff in contrib/, I'm not sure to what degree it applies.
diff-highlight is pretty standalone for instance. I guess it _could_ be
broken by a public-API change in Git, but I find it pretty unlikely.

Of course, you have to bear the burden of (A) changing the way
scalar uses the API, or (B) participating in the design of the
change to the API that may break scalar's use so that everybody
including scalar would be happy, or both.  It's not like I am
responsible for everything that happens in the tree, and it is our
shared responsibility to maintain the health of the codebase.  It is
not limited to stuff inside or outside contrib/.

There are projects that want to use libgit.a by binding us as a
submodule and without interacting with us very much.  And they are
on their own when we change the internals.  Do you mean that you
want to make scalar into the same status as they are?

I kind of thought that final paragraph was the plan, at least to start
with.

-Peff

On Tue, Dec 14, 2021 at 08:16:26AM -0500, Jeff King wrote:

quoted

There are projects that want to use libgit.a by binding us as a
submodule and without interacting with us very much.  And they are
on their own when we change the internals.  Do you mean that you
want to make scalar into the same status as they are?

I kind of thought that final paragraph was the plan, at least to start
with.

Oh, and just to be clear: I am really OK with either direction. I'm only
claiming that I think both approaches are self-consistent and are making
a tradeoff (finding bugs earlier, versus shifting burden of bug-fixing
around).

-Peff