[PATCH 0/1] Makefile: add prove and coverage-prove targets

[PATCH 0/1] Makefile: add prove and coverage-prove targets

[Derrick Stolee via GitGitGadget]

Sometimes there are test failures in the 'pu' branch. This is somewhat
expected for a branch that takes the very latest topics under development,
and those sometimes have semantic conflicts that only show up during test
runs. This also can happen when running the test suite with different
GIT_TEST_* environment variables that interact in unexpected ways.

This causes a problem for the test coverage reports, as the typical 'make
coverage-test coverage-report' run halts at the first failed test. If that
test is early in the suite, then many valuable tests are not exercising the
code and the coverage report becomes noisy with false positives.

This patch adds two targets to the Makefile: 'prove' and 'coverage-prove'.
The idea is to use the 'prove' tool to run the test suite, as it will track
failed tests but continue running the full suite even with a failure.

If/when this merges down, I will use this new target for the automation
around the test coverage reports.

Thanks, -Stolee

Derrick Stolee (1):
  Makefile: add prove and coverage-prove targets

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


base-commit: 0d0ac3826a3bbb9247e39e12623bbcfdd722f24c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-114%2Fderrickstolee%2Fcoverage-prove-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-114/derrickstolee/coverage-prove-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/114
-- 
gitgitgadget

[PATCH 1/1] Makefile: add prove and coverage-prove targets

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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 <dstolee@microsoft.com>
---
 Makefile | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Makefile b/Makefile
index 1a44c811aa..ec886635ae 100644
--- a/Makefile
+++ b/Makefile
@@ -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; \
-- 
gitgitgadget

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Johannes Schindelin]

Hi Stolee,

On Tue, 29 Jan 2019, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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.

The rationale, and the diff (after reading what `test` and `coverage-test`
do), make a lot of sense to me.

Thanks,
Dscho

> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Makefile | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 1a44c811aa..ec886635ae 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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; \
> -- 
> gitgitgadget
> 

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[SZEDER Gábor]

On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:
> 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.

I think the cover letter would be a better commit message.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Makefile | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 1a44c811aa..ec886635ae 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2665,6 +2665,9 @@ export TEST_NO_MALLOC_CHECK
>  test: all
>  	$(MAKE) -C t/ all
>  
> +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.

>  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

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.

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Derrick Stolee]

On 1/29/2019 10:58 AM, SZEDER Gábor wrote:
> On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:
>> +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.

Thanks!

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

Thanks for finding this!
 
> 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.

Thanks,
-Stolee

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Jeff King]

On Tue, Jan 29, 2019 at 11:37:34AM -0500, Derrick Stolee wrote:

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

I think doing:

  make GIT_PROVE_OPTS="$(GIT_PROVE_OPTS) -j1"

should be sufficient to pass through most of the options but override
-j, since prove does the usual left-to-right "last one wins" behavior
for its options.

-Peff

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[SZEDER Gábor]

On Tue, Jan 29, 2019 at 04:58:27PM +0100, SZEDER Gábor wrote:
> On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:
> > @@ -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.

So this text is really dramatic and implies (to me, at least), that
parallelized coverage builds just don't work.  I've just run a
coverage build with this patch and my usual GIT_PROVE_OPTS containing
'-j4', and the tests went well and the generated report looks good,
too (I don't know how it's supposed to look, but at least I didn't
notice anything obviously wrong with it).  However, this might mean
nothing, because further digging turned up the follow paragraph in
901c369af5 (Support coverage testing with GCC/gcov, 2009-02-19):

    The tests are run serially (with -j1).  The coverage code should
    theoretically allow concurrent access to its data files, but the
    author saw random test failures.  Obviously this could be
    improved.

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.


[1] https://public-inbox.org/git/200902191512.16755.trast@student.ethz.ch/

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Derrick Stolee]

On 1/29/2019 12:34 PM, SZEDER Gábor wrote:
> 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.

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.

Thanks!
-Stolee

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Derrick Stolee]

On 1/29/2019 1:10 PM, Derrick Stolee wrote:
> On 1/29/2019 12:34 PM, SZEDER Gábor wrote:
>> 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.
> 
> 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.

Thanks,
-Stolee
 

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[SZEDER Gábor]

On Tue, Jan 29, 2019 at 03:49:58PM -0500, Derrick Stolee wrote:
> On 1/29/2019 1:10 PM, Derrick Stolee wrote:
> > On 1/29/2019 12:34 PM, SZEDER Gábor wrote:
> >> 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.
> > 
> > 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.

That failure is not coverage-related, but as it turned out 9e5da3d055
(add: use separate ADD_CACHE_RENORMALIZE flag, 2019-01-17) made t0025
rather flaky:

  https://public-inbox.org/git/20190129213533.GE13764@szeder.dev/

When reading those old commit messages and discussions in the
afternoon, I was wondering what "random test failures" actually meant,
since it was not stated explicitly that it was coverage-related.  For
all we know it could have been "general" test flakiness that happened
to manifest under the higher load of a parallel test run.

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Jeff King]

On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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.

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?

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

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.

-Peff

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Derrick Stolee]

On 1/29/2019 11:00 AM, Jeff King wrote:
> On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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.
> 
> 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?

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?

>> @@ -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
>> +
> 
> 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?

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

Thanks,
-Stolee

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Jeff King]

On Tue, Jan 29, 2019 at 11:35:41AM -0500, Derrick Stolee wrote:

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

Another option would be to relay "-k" from the caller. I think it's not
enough to just use $(MAKE), but if you use $(MAKE) $(MAKEFLAGS), then
running "make -k coverage-test" from your coverage script would (I
think) do what you want.

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

Yeah, that matches my understanding. I don't think you'd have to deal
with that failure code manually for coverage-report because it does not
depend on coverage-test (but obviously if you did "make coverage-test &&
make coverage-report", the "&&" needs to become a semicolon).

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

Yeah, that works. I typically do run prove, and I do run the tests from
the root, but DEFAULT_TEST_TARGET already makes it all work for me. So
yeah, I think it's fine to leave off the prove target until somebody
actually wants it.

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

Ah, right, I vaguely recall that now.

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

Makes sense.

-Peff

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 29 2019, Jeff King wrote:

> On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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.
>
> 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?

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Jeff King]

On Tue, Jan 29, 2019 at 10:03:46PM +0100, Ævar Arnfjörð Bjarmason wrote:

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

Certainly no argument from me personally, but I do wonder if anybody
uses the old one. Maybe we could mark it with a deprecation notice or
something. I'd be happy to do a straw poll at the contributor summit (or
on the list, if anybody reads this ;) ).

-Peff

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

Hi Ævar,

On Tue, 29 Jan 2019, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Jan 29 2019, Jeff King wrote:
> 
> > On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via
> > GitGitGadget wrote:
> >
> >> From: Derrick Stolee <dstolee@microsoft.com>
> >>
> >> 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.
> >
> > 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.

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.

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

Ciao,
Dscho

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 30 2019, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Tue, 29 Jan 2019, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Jan 29 2019, Jeff King wrote:
>>
>> > On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via
>> > GitGitGadget wrote:
>> >
>> >> From: Derrick Stolee <dstolee@microsoft.com>
>> >>
>> >> 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.
>> >
>> > 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.
>
> 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.

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

Let's be clear about what "prove support" means.

When I added support for "prove" I was really adding support for TAP
which is a standardized test output protocol: https://testanything.org/

It just so happens that "prove" is the most ubiquitous implementation,
but there's plenty of others: https://testanything.org/consumers.html

So hard-depending on "prove" in no way ties us to that particular tool
forever. We'd just do away with ferrying information via a side-channel
(*.counts files) that we also get from its output.

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

*Nod*

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4553 bytes --]

Hi Ævar,

On Wed, 30 Jan 2019, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Jan 30 2019, Johannes Schindelin wrote:
> 
> > On Tue, 29 Jan 2019, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Tue, Jan 29 2019, Jeff King wrote:
> >>
> >> > On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via
> >> > GitGitGadget wrote:
> >> >
> >> >> From: Derrick Stolee <dstolee@microsoft.com>
> >> >>
> >> >> 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.
> >> >
> >> > 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.
> >
> > 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.

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

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

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

Ciao,
Dscho

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 30 2019, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Wed, 30 Jan 2019, Ævar Arnfjörð Bjarmason wrote:
>
>> On Wed, Jan 30 2019, Johannes Schindelin wrote:
>>
>> > On Tue, 29 Jan 2019, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> On Tue, Jan 29 2019, Jeff King wrote:
>> >>
>> >> > On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via
>> >> > GitGitGadget wrote:
>> >> >
>> >> >> From: Derrick Stolee <dstolee@microsoft.com>
>> >> >>
>> >> >> 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.
>> >> >
>> >> > 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.
>> >
>> > 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.
>
>> 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.
>
>> 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.
>
>> > 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.

Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

Hi Ævar,

On Wed, 30 Jan 2019, Ævar Arnfjörð Bjarmason wrote:

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

Hmm. You're right, I basically see a range of 5.17-5.27 seconds, all well
in the noise.

So apart from the size for Perl (which accounts for about a third of the
subset of the Git for Windows SDK we have to download into each and every
CI run, multiple times) it does not hurt that much at the moment, you're
right.

Still, I would like to get away from our reliance on Perl in the test
suite. It does not make sense to require Perl even with NO_PERL.

Ciao,
Dscho

[PATCH v2 0/1] Makefile: add prove and coverage-prove targets

[Derrick Stolee via GitGitGadget]

Sometimes there are test failures in the 'pu' branch. This is somewhat
expected for a branch that takes the very latest topics under development,
and those sometimes have semantic conflicts that only show up during test
runs. This also can happen when running the test suite with different
GIT_TEST_* environment variables that interact in unexpected ways.

This causes a problem for the test coverage reports, as the typical 'make
coverage-test coverage-report' run halts at the first failed test. If that
test is early in the suite, then many valuable tests are not exercising the
code and the coverage report becomes noisy with false positives.

This patch adds two targets to the Makefile: 'prove' and 'coverage-prove'.
The idea is to use the 'prove' tool to run the test suite, as it will track
failed tests but continue running the full suite even with a failure.

If/when this merges down, I will use this new target for the automation
around the test coverage reports.

Updates in V2:

 * Dropped the 'prove' target
   
   
 * Append '-j1' to GIT_PROVE_OPTS
   
   
 * Commit message tweaks.

Derrick Stolee (1):
  Makefile: add coverage-prove target

 Makefile | 5 +++++
 1 file changed, 5 insertions(+)


base-commit: 0d0ac3826a3bbb9247e39e12623bbcfdd722f24c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-114%2Fderrickstolee%2Fcoverage-prove-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-114/derrickstolee/coverage-prove-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/114

Range-diff vs v1:

 1:  294187c696 < -:  ---------- Makefile: add prove and coverage-prove targets
 -:  ---------- > 1:  af810fad97 Makefile: add coverage-prove target

-- 
gitgitgadget

[PATCH v2 1/1] Makefile: add coverage-prove target

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Sometimes there are test failures in the 'pu' branch. This
is somewhat expected for a branch that takes the very latest
topics under development, and those sometimes have semantic
conflicts that only show up during test runs. This also can
happen when running the test suite with different GIT_TEST_*
environment variables that interact in unexpected ways

This causes a problem for the test coverage reports, as
the typical 'make coverage-test coverage-report' run halts
at the first failed test. If that test is early in the
suite, then many valuable tests are not exercising the code
and the coverage report becomes noisy with false positives.

Add a new 'coverage-prove' target to the Makefile,
modeled after the 'coverage-test' target. This compiles
the source using the coverage flags, then runs the test
suite using the 'prove' tool. Since the coverage
machinery is not thread-safe, enforce that the tests
are run in sequence by appending '-j1' to GIT_PROVE_OPTS.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Makefile | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Makefile b/Makefile
index 1a44c811aa..23d8730482 100644
--- a/Makefile
+++ b/Makefile
@@ -3077,6 +3077,11 @@ 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 GIT_PROVE_OPTS="$(GIT_PROVE_OPTS) -j1" \
+		-j1 test
+
 coverage-report:
 	$(QUIET_GCOV)for dir in $(object_dirs); do \
 		$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
-- 
gitgitgadget