Makefile: add prove and coverage-prove targets

From: Derrick Stolee <[email protected]>

When running the test suite for code coverage using 'make coverage-test', a single test failure stops the test suite from completing. This leads to significant undercounting of covered blocks.

Add two new targets to the Makefile:

* 'prove' runs the test suite using 'prove'.

* 'coverage-prove' compiles the source using the coverage flags, then runs the test suite using 'prove'.

These targets are modeled after the 'test' and 'coverage-test' targets.

Signed-off-by Derrick Stolee <[email protected]>

---
 Makefile | 7 +++++++
 1 file changed, 7 insertions(+)

Makefile +7 -0

index 1a44c811aa..ec886635ae 100644
@@ -2665,6 +2665,9 @@ export TEST_NO_MALLOC_CHECK
 test: all
 	$(MAKE) -C t/ all
 
+prove: all
+	$(MAKE) -C t/ prove
+
 perf: all
 	$(MAKE) -C t/perf/ all
 

@@ -3077,6 +3080,10 @@ coverage-test: coverage-clean-results coverage-compile
 	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
 		DEFAULT_TEST_TARGET=test -j1 test
 
+coverage-prove: coverage-clean-results coverage-compile
+	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
+		DEFAULT_TEST_TARGET=prove -j1 prove
+
 coverage-report:
 	$(QUIET_GCOV)for dir in $(object_dirs); do \
 		$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \

Signature · 1 line

gitgitgadget

When running the test suite for code coverage using 'make coverage-test', a single test failure stops the test suite from completing. This leads to significant undercounting of covered blocks.

Add two new targets to the Makefile:

* 'prove' runs the test suite using 'prove'.

* 'coverage-prove' compiles the source using the coverage flags, then runs the test suite using 'prove'.

These targets are modeled after the 'test' and 'coverage-test' targets.

Signed-off-by Derrick Stolee <[email protected]>

---
 Makefile | 7 +++++++
 1 file changed, 7 insertions(+)

Makefile +0 -0

index 1a44c811aa..ec886635ae 100644

Makefile export TEST_NO_MALLOC_CHECK · line 2670

Quoted 3 lines · Derrick Stolee via GitGitGadget

Makefile +3 -0

+prove: all
+	$(MAKE) -C t/ prove
+

You don't need this 'prove' target in the "main" Makefile, because 'make test' will run the test suite using DEFAULT_TEST_TARGET anyway.

Makefile coverage-test: coverage-clean-results coverage-compile · line 3085

Quoted 3 lines

Makefile +3 -0

+coverage-prove: coverage-clean-results coverage-compile
+	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
+		DEFAULT_TEST_TARGET=prove -j1 prove

First I was wondering why do you need a dedicated 'coverage-prove' target, instead of letting DEFAULT_TEST_TARGET from the environment or from 'config.mak' do its thing. But then I noticed in the hunk context, that, for some reason, the 'coverage-test' target hardcoded 'DEFAULT_TEST_TARGET=test -j1'. Then I was wondering why would it want to do that, and stumbled upon commit c14cc77c11:

coverage: set DEFAULT_TEST_TARGET to avoid using prove

If the user sets DEFAULT_TEST_TARGET=prove in his config.mak, that carries over into the coverage tests. Which is really bad if he also sets GIT_PROVE_OPTS=-j<..> as that completely breaks the coverage runs.

Instead of attempting to mess with the GIT_PROVE_OPTS, just force the test target to 'test' so that we run under make, like we intended all along.

I'm afraid that this issue would badly affect 'coverage-prove' as well (I didn't try). Or if doesn't (anymore?), then that should be mentioned in the commit message, and then perhaps it's time to remove that '-j1' from the 'coverage-test' target as well.

Quoted 4 lines

I'm afraid that this issue would badly affect 'coverage-prove' as well (I didn't try). Or if doesn't (anymore?), then that should be mentioned in the commit message, and then perhaps it's time to remove that '-j1' from the 'coverage-test' target as well.

I'll fix this by forcing an update to GIT_PROVE_OPTS. It does limit our ability to use GIT_PROVE_OPTS as a pass-through, but at least this new target will have that assumption built in.

Makefile coverage-test: coverage-clean-results coverage-compile · line 3085

Quoted 7 lines · Derrick Stolee via GitGitGadget

Makefile +3 -0

@@ -3077,6 +3080,10 @@ coverage-test: coverage-clean-results coverage-compile
 	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
 		DEFAULT_TEST_TARGET=test -j1 test
 
+coverage-prove: coverage-clean-results coverage-compile
+	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
+		DEFAULT_TEST_TARGET=prove -j1 prove

First I was wondering why do you need a dedicated 'coverage-prove' target, instead of letting DEFAULT_TEST_TARGET from the environment or from 'config.mak' do its thing. But then I noticed in the hunk context, that, for some reason, the 'coverage-test' target hardcoded 'DEFAULT_TEST_TARGET=test -j1'. Then I was wondering why would it want to do that, and stumbled upon commit c14cc77c11:

coverage: set DEFAULT_TEST_TARGET to avoid using prove

If the user sets DEFAULT_TEST_TARGET=prove in his config.mak, that carries over into the coverage tests. Which is really bad if he also sets GIT_PROVE_OPTS=-j<..> as that completely breaks the coverage runs.

On Tue, Jan 29, 2019 at 04:58:27PM +0100, SZEDER Gábor wrote:
And in the related email discussion [1]:

But even though the docs claim it [-j<N>] should be possible, I've been getting "random" test failures when compiled with coverage support, that went away with -j1. So the tests still run with -j1, as with the first version of the series.

So it doesn't seem to be that bad after all, because it's not "completely breaks" but "random test failures". Still far from ideal, but the original coverage patch is just about 3 weeks short of a decade old, so maybe things have improved since then, and it'd be worth a try to leave GIT_PROVE_OPTS as is and see what happens.

Quoted 8 lines · SZEDER Gábor

On Tue, Jan 29, 2019 at 04:58:27PM +0100, SZEDER Gábor wrote:
And in the related email discussion [1]:

But even though the docs claim it [-j<N>] should be possible, I've been getting "random" test failures when compiled with coverage support, that went away with -j1. So the tests still run with -j1, as with the first version of the series.

Quoted 5 lines

So it doesn't seem to be that bad after all, because it's not "completely breaks" but "random test failures". Still far from ideal, but the original coverage patch is just about 3 weeks short of a decade old, so maybe things have improved since then, and it'd be worth a try to leave GIT_PROVE_OPTS as is and see what happens.

It would certainly be nice if the build time could be reduced through parallel test runs. I've kicked off a build using GIT_PROVE_OPTS="-j12" to see what happens.

Quoted 15 lines · Derrick Stolee

Quoted 8 lines · SZEDER Gábor

On Tue, Jan 29, 2019 at 04:58:27PM +0100, SZEDER Gábor wrote:
And in the related email discussion [1]:

But even though the docs claim it [-j<N>] should be possible, I've been getting "random" test failures when compiled with coverage support, that went away with -j1. So the tests still run with -j1, as with the first version of the series.

Quoted 5 lines

So it doesn't seem to be that bad after all, because it's not "completely breaks" but "random test failures". Still far from ideal, but the original coverage patch is just about 3 weeks short of a decade old, so maybe things have improved since then, and it'd be worth a try to leave GIT_PROVE_OPTS as is and see what happens.

Quoted 3 lines

It would certainly be nice if the build time could be reduced through parallel test runs. I've kicked off a build using GIT_PROVE_OPTS="-j12" to see what happens.

I did get a failed test with this run:

t0025-crlf-renormalize.sh (Wstat: 256 Tests: 3 Failed: 1)
Failed test: 2
Non-zero exit status: 1

This was on the 'jch' branch, and an equivalent build with sequential execution did not have this failure. That's flaky enough for me to stick to sequential runs.

From: Derrick Stolee <[email protected]>

When running the test suite for code coverage using 'make coverage-test', a single test failure stops the test suite from completing. This leads to significant undercounting of covered blocks.

Add two new targets to the Makefile:

* 'prove' runs the test suite using 'prove'.

* 'coverage-prove' compiles the source using the coverage flags, then runs the test suite using 'prove'.

These targets are modeled after the 'test' and 'coverage-test' targets.

Quoted 9 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

When running the test suite for code coverage using 'make coverage-test', a single test failure stops the test suite from completing. This leads to significant undercounting of covered blocks.

Add two new targets to the Makefile:

Quoted 8 lines

* 'prove' runs the test suite using 'prove'.

* 'coverage-prove' compiles the source using the coverage flags, then runs the test suite using 'prove'.

These targets are modeled after the 'test' and 'coverage-test' targets.

I think these are reasonable to have (and I personally much prefer "prove" to the raw "make test" output anyway).

For people who don't have "prove" available, I think they could just do "make -k test" to make sure the full suite runs. Should we perhaps be doing that automatically in the sub-make run by coverage-test?

Quoted 4 lines

Makefile +0 -0

@@ -3077,6 +3080,10 @@ coverage-test: coverage-clean-results coverage-compile
 	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
 		DEFAULT_TEST_TARGET=test -j1 test
 

Makefile coverage-test: coverage-clean-results coverage-compile · line 3086

Quoted 4 lines

Makefile +4 -0

+coverage-prove: coverage-clean-results coverage-compile
+	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
+		DEFAULT_TEST_TARGET=prove -j1 prove
+

You probably don't need to override DEFAULT_TEST_TARGET here, since the "prove" target doesn't look at it. Likewise, "-j1" probably does nothing here, since prove itself is a single target.

I'm not sure why we want to enforce -j1 for these targets, but if it's important to do so for the prove case, as well, you'd need to add it to GIT_PROVE_OPTS.

Quoted 3 lines

For people who don't have "prove" available, I think they could just do "make -k test" to make sure the full suite runs. Should we perhaps be doing that automatically in the sub-make run by coverage-test?

I wanted to avoid changing the existing behavior, if I could. But, if we can reasonably assume that anyone running 'make coverage-test' wants to run the full suite even with failures, then that's fine by me.

I see from the make docs that '-k' will still result in an error code at the end of the command, so no automation would result in an incorrect response to a failed test. Am I correct?

Quoted 8 lines

Makefile coverage-test: coverage-clean-results coverage-compile · line 3086

Quoted 4 lines

Makefile +4 -0

+coverage-prove: coverage-clean-results coverage-compile
+	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
+		DEFAULT_TEST_TARGET=prove -j1 prove
+

You probably don't need to override DEFAULT_TEST_TARGET here, since the "prove" target doesn't look at it. Likewise, "-j1" probably does nothing here, since prove itself is a single target.

As Szeder mentioned, I can probably just drop the 'prove' target and use DEFAULT_TEST_TARGET instead. Or do we think anyone will want to use 'make prove' from root?

Quoted 3 lines

I'm not sure why we want to enforce -j1 for these targets, but if it's important to do so for the prove case, as well, you'd need to add it to GIT_PROVE_OPTS.

The '-j1' is necessary because the coverage data is collected in a way that is not thread-safe. Our compile options also force single-threaded behavior.

I'll specifically override GIT_PROVE_OPTS here to force -j1, but also send -j1 to the 'make' command, too.

Quoted 15 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

When running the test suite for code coverage using 'make coverage-test', a single test failure stops the test suite from completing. This leads to significant undercounting of covered blocks.

Add two new targets to the Makefile:

* 'prove' runs the test suite using 'prove'.

* 'coverage-prove' compiles the source using the coverage flags, then runs the test suite using 'prove'.

Quoted 2 lines

These targets are modeled after the 'test' and 'coverage-test' targets.

I think these are reasonable to have (and I personally much prefer "prove" to the raw "make test" output anyway).

Quoted 2 lines

I think these are reasonable to have (and I personally much prefer "prove" to the raw "make test" output anyway).

I wonder if anyone would mind if we removed the non-prove path.

When I added it in 5099b99d25 ("test-lib: Adjust output to be valid TAP format", 2010-06-24) there were still some commonly shipped OS's that had a crappy old "prove", but now almost a decade later that's not a practical problem, and it's installed by default with perl, and we already depend on perl for the tests.

I don't feel strongly about it, but it would allow us to prune some login in the test library / Makefile.

Maybe something for a show of hands at the contributor summit?

Quoted 18 lines · Jeff King

Quoted 15 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

When running the test suite for code coverage using 'make coverage-test', a single test failure stops the test suite from completing. This leads to significant undercounting of covered blocks.

Add two new targets to the Makefile:

* 'prove' runs the test suite using 'prove'.

* 'coverage-prove' compiles the source using the coverage flags, then runs the test suite using 'prove'.

Quoted 5 lines

Quoted 2 lines

These targets are modeled after the 'test' and 'coverage-test' targets.

I think these are reasonable to have (and I personally much prefer "prove" to the raw "make test" output anyway).

I wonder if anyone would mind if we removed the non-prove path.

When I added it in 5099b99d25 ("test-lib: Adjust output to be valid TAP format", 2010-06-24) there were still some commonly shipped OS's that had a crappy old "prove", but now almost a decade later that's not a practical problem, and it's installed by default with perl, and we already depend on perl for the tests.

I don't feel strongly about it, but it would allow us to prune some login in the test library / Makefile.

Maybe something for a show of hands at the contributor summit?

Hi Ævar,

Quoted 28 lines · Ævar Arnfjörð Bjarmason

Quoted 18 lines · Jeff King

Quoted 15 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

When running the test suite for code coverage using 'make coverage-test', a single test failure stops the test suite from completing. This leads to significant undercounting of covered blocks.

Add two new targets to the Makefile:

* 'prove' runs the test suite using 'prove'.

* 'coverage-prove' compiles the source using the coverage flags, then runs the test suite using 'prove'.

Quoted 5 lines

Quoted 2 lines

These targets are modeled after the 'test' and 'coverage-test' targets.

I think these are reasonable to have (and I personally much prefer "prove" to the raw "make test" output anyway).

I wonder if anyone would mind if we removed the non-prove path.

Quoted 5 lines

When I added it in 5099b99d25 ("test-lib: Adjust output to be valid TAP format", 2010-06-24) there were still some commonly shipped OS's that had a crappy old "prove", but now almost a decade later that's not a practical problem, and it's installed by default with perl, and we already depend on perl for the tests.

It's not only about crappy old prove, it is also about requiring Perl (and remember, Perl is not really native in Git for Windows' case;

I still have a hunch that we could save on time *dramatically* by simply running through regular make rather than through prove).

I did start to implement a parallel test runner for use with BusyBox-based MinGit, but dropped the ball on that front before I could satisfy myself that this is robust enough. Once it *is* robust enough, we could even replace the entire prove support with a native, test-tool driven test framework.

Quoted 4 lines

I don't feel strongly about it, but it would allow us to prune some login in the test library / Makefile.

Maybe something for a show of hands at the contributor summit?

Sure, let's put it up for discussion.

Quoted 36 lines · Johannes Schindelin

Quoted 33 lines · Ævar Arnfjörð Bjarmason

Quoted 18 lines · Jeff King

Quoted 15 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

When running the test suite for code coverage using 'make coverage-test', a single test failure stops the test suite from completing. This leads to significant undercounting of covered blocks.

Add two new targets to the Makefile:

* 'prove' runs the test suite using 'prove'.

* 'coverage-prove' compiles the source using the coverage flags, then runs the test suite using 'prove'.

Quoted 5 lines

Quoted 2 lines

These targets are modeled after the 'test' and 'coverage-test' targets.

I think these are reasonable to have (and I personally much prefer "prove" to the raw "make test" output anyway).

I wonder if anyone would mind if we removed the non-prove path.

When I added it in 5099b99d25 ("test-lib: Adjust output to be valid TAP format", 2010-06-24) there were still some commonly shipped OS's that had a crappy old "prove", but now almost a decade later that's not a practical problem, and it's installed by default with perl, and we already depend on perl for the tests.

Quoted 2 lines

It's not only about crappy old prove, it is also about requiring Perl (and remember, Perl is not really native in Git for Windows' case;

We require perl now for testing, NO_PERL is just for the installed version of git.

If you change the various test-lib.sh and test-lib-functions.sh that unconditionally uses "perl" or "$PERL_PATH" hundreds/thousands (didn't take an exact count, just watched fail scroll by) tests fail.

So my assumption is that anyone running the tests now has perl anyway, and thus a further hard dependency on it won't hurt anything.

Quoted 2 lines

I still have a hunch that we could save on time *dramatically* by simply running through regular make rather than through prove).

My hunch is that on the OS's where this would matter (e.g. Windows) the overhead is mainly spawning the processes, and it doesn't matter if it's make or perl doing the spawning, but I have nothing to back that up...

Hi Ævar,

Quoted 38 lines · Ævar Arnfjörð Bjarmason

Quoted 36 lines · Johannes Schindelin

Quoted 33 lines · Ævar Arnfjörð Bjarmason

Quoted 18 lines · Jeff King

Quoted 15 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

When running the test suite for code coverage using 'make coverage-test', a single test failure stops the test suite from completing. This leads to significant undercounting of covered blocks.

Add two new targets to the Makefile:

* 'prove' runs the test suite using 'prove'.

* 'coverage-prove' compiles the source using the coverage flags, then runs the test suite using 'prove'.

Quoted 5 lines

Quoted 2 lines

These targets are modeled after the 'test' and 'coverage-test' targets.

I think these are reasonable to have (and I personally much prefer "prove" to the raw "make test" output anyway).

I wonder if anyone would mind if we removed the non-prove path.

When I added it in 5099b99d25 ("test-lib: Adjust output to be valid TAP format", 2010-06-24) there were still some commonly shipped OS's that had a crappy old "prove", but now almost a decade later that's not a practical problem, and it's installed by default with perl, and we already depend on perl for the tests.

Quoted 5 lines

Quoted 2 lines

It's not only about crappy old prove, it is also about requiring Perl (and remember, Perl is not really native in Git for Windows' case;

We require perl now for testing, NO_PERL is just for the installed version of git.

Which is confusing, if you want to put it nicely.

Quoted 3 lines

If you change the various test-lib.sh and test-lib-functions.sh that unconditionally uses "perl" or "$PERL_PATH" hundreds/thousands (didn't take an exact count, just watched fail scroll by) tests fail.

I know. Oh boy, I know.

But we do not have to keep that status quo, nor do we have to make it worse.

It would not surprise me in the least if we could accelerate our entire test suite by reducing our heavy reliance on scripting (including Perl) to the point that it really takes too little time *not* to run. (Right now, if you are on Windows, you better think twice before you start the test suite, it will easily take over 3h (!!!) to run in a regular developer setup. Even on a regular Mac, I would think twice before starting the run that blocks my machine for easily 20 minutes straight. Needless to say that few developers, if any, use it to validate their patches, in particular on Windows. Meaning: for all real purposes, the test suite is nearly useless on Windows.)

So let's not bake *even more* Perl usage into our test suite. Thanks.

Quoted 2 lines

So my assumption is that anyone running the tests now has perl anyway, and thus a further hard dependency on it won't hurt anything.

By that token, the effort to turn many a script into a built-in for better performance and substantially better error checking would be totally nonsensical. "Because anyone running Git used those scripts anyway, so making them a hard dependency won't hurt anything"?

I do not believe even a fraction of a second that that effort is nonsensical. Just like I do not believe even a fraction of a second that it makes sense for our test suite to rely on scripting so much. Or for us to make that reliance even bigger, for that matter.

Quoted 6 lines

Quoted 2 lines

I still have a hunch that we could save on time *dramatically* by simply running through regular make rather than through prove).

My hunch is that on the OS's where this would matter (e.g. Windows) the overhead is mainly spawning the processes, and it doesn't matter if it's make or perl doing the spawning, but I have nothing to back that up...

I have at least the experience of several thousands runs of the test suite on Windows, together with a couple dozen hours spent recently *just* on making the CI of GitGitGadget at least bearable.

So I do not quite understand why you offered a contrary opinion when you have nothing to back it up.

I mean, I would really like to have an informed discussion with you, to benefit from your skills and from your experience to make the entire design of our test suite better (there is so much room for improvement, we should really be able to put together our knowledge to enhance it). It needs to be based on facts, of course.

Let's get some numbers then. On master, go to the "t" directory and run this:

    for f in t[0-9]*.sh; do (echo '#!/bin/sh' && echo "echo ok 1 $f" && echo sleep 1 && echo echo 1..1) >$f; done

That effectively turns all our tests into a "hello world" with a sleep of 1 second.

Then run both:

    time prove -j12 t00[0-9]*.sh

And:

    time make -j12 t00[0-9]*.sh

For some value of -j12 and t00[0-9]*.sh. In my testing "make" is a bit faster, but not by any amount that would matter when this is run for real.