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

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 29, 2019, 16:35 UTC
Message-ID: <bb17112d-2869-741f-de13-3971a995032e@gmail.com>
In-Reply-To: <20190129160030.GA7083@sigill.intra.peff.net>

On 1/29/2019 11:00 AM, Jeff King wrote:

Show 26 quoted lines

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

Show 12 quoted lines

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