threads / rfc / 25442

RFC patchMakefile: new prove target for running the tests with TAP

Subject: [RFC PATCH] Makefile: new prove target for running the tests with TAP

## tl;dr

10 messages between Oct 14, 2010 and Oct 29, 2010. Diffs are folded; open one to read it.

replies: 9people: 5as markdown or json

Michael J Gruber· Oct 14, 2010, 08:53 UTC · lore

Introduce a new make target "prove" which runs the tests via "prove" (or $(PROVE) if set) so that one does not have to cd around any more. One can simply use "make prove" for "make test" or go wild as in:

GIT_SKIP_TESTS='t[0-4]??? t91?? t9200.8' GIT_PROVE_OPTS="-j9 -v" GIT_TEST_OPTS="--verbose --debug" make prove
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
RFC also because of lack of doc, and:
Currently, one can do
make -C t t6010-merge-base.sh
or even
make -C t t601*

which is cool but undocumented. If we want this with prove it requires more effort, or shoving prove into the $(T) target rule which is doable, of course. I'm just wondering whether it's accepted to introduce a specific rule for prove at all.

 Makefile   |    3 +++
 t/Makefile |    5 +++++
 2 files changed, 8 insertions(+), 0 deletions(-)
Show changes to 2 files +8 −0

Makefile, t/Makefile

diff --git a/Makefile b/Makefile
index 1f1ce04..3478515 100644
--- a/Makefile
+++ b/Makefile
@@ -2046,6 +2046,9 @@ export NO_SVN_TESTS
 test: all
 	$(MAKE) -C t/ all
 
+prove: all
+	$(MAKE) -C t/ prove
+
 test-ctype$X: ctype.o
 
 test-date$X: date.o ctype.o
diff --git a/t/Makefile b/t/Makefile
index c7baefb..e4661c0 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -11,6 +11,7 @@ SHELL_PATH ?= $(SHELL)
 PERL_PATH ?= /usr/bin/perl
 TAR ?= $(TAR)
 RM ?= rm -f
+PROVE ?= prove
 
 # Shell quote;
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
@@ -21,6 +22,10 @@ TSVN = $(wildcard t91[0-9][0-9]-*.sh)
 all: pre-clean
 	$(MAKE) aggregate-results-and-cleanup
 
+prove: pre-clean
+	@echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	$(MAKE) clean
+
 $(T):
 	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
 

-- 
1.7.3.1.184.g5b1fd
Ævar Arnfjörð Bjarmason· Oct 14, 2010, 09:05 UTC · re: Michael J Gruber · lore

Re: [RFC PATCH] Makefile: new prove target for running the tests with TAP

On Thu, Oct 14, 2010 at 08:53, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 22 quoted lines
> Introduce a new make target "prove" which runs the tests via "prove" (or
> $(PROVE) if set) so that one does not have to cd around any more. One
> can simply use "make prove" for "make test" or go wild as in:
>
> GIT_SKIP_TESTS='t[0-4]??? t91?? t9200.8' GIT_PROVE_OPTS="-j9 -v" GIT_TEST_OPTS="--verbose --debug" make prove
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> RFC also because of lack of doc, and:
>
> Currently, one can do
>
> make -C t t6010-merge-base.sh
>
> or even
>
> make -C t t601*
>
> which is cool but undocumented. If we want this with prove it requires more
> effort, or shoving prove into the $(T) target rule which is doable, of course.
> I'm just wondering whether it's accepted to introduce a specific rule for prove
> at all.

I like the rationale behind this, but fwiw. I already had a patch that was ejected for this:

    http://article.gmane.org/gmane.comp.version-control.git/146566
Quoth Junio:
    "Besides, "make -j15 test" from the toplevel already runs the
    tests in parallel.  I don't see much point in this change."

But I'd like to have it so that I could `make && make test_harness` and get readable test output.

But we should probably be using t/harness to run them, not prove(1). Then we would make the test_harness target respect the HARNESS_OPTIONS variable, which I e.g. have set to HARNESS_OPTIONS=j9 on my system.

Show 46 quoted lines
>
>  Makefile   |    3 +++
>  t/Makefile |    5 +++++
>  2 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 1f1ce04..3478515 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2046,6 +2046,9 @@ export NO_SVN_TESTS
>  test: all
>        $(MAKE) -C t/ all
>
> +prove: all
> +       $(MAKE) -C t/ prove
> +
>  test-ctype$X: ctype.o
>
>  test-date$X: date.o ctype.o
> diff --git a/t/Makefile b/t/Makefile
> index c7baefb..e4661c0 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -11,6 +11,7 @@ SHELL_PATH ?= $(SHELL)
>  PERL_PATH ?= /usr/bin/perl
>  TAR ?= $(TAR)
>  RM ?= rm -f
> +PROVE ?= prove
>
>  # Shell quote;
>  SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
> @@ -21,6 +22,10 @@ TSVN = $(wildcard t91[0-9][0-9]-*.sh)
>  all: pre-clean
>        $(MAKE) aggregate-results-and-cleanup
>
> +prove: pre-clean
> +       @echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
> +       $(MAKE) clean
> +
>  $(T):
>        @echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
>
> --
> 1.7.3.1.184.g5b1fd
>
>
Michael J Gruber· Oct 14, 2010, 09:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC PATCH] Makefile: new prove target for running the tests with TAP

Ævar Arnfjörð Bjarmason venit, vidit, dixit 14.10.2010 11:05:
Show 35 quoted lines
> On Thu, Oct 14, 2010 at 08:53, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>> Introduce a new make target "prove" which runs the tests via "prove" (or
>> $(PROVE) if set) so that one does not have to cd around any more. One
>> can simply use "make prove" for "make test" or go wild as in:
>>
>> GIT_SKIP_TESTS='t[0-4]??? t91?? t9200.8' GIT_PROVE_OPTS="-j9 -v" GIT_TEST_OPTS="--verbose --debug" make prove
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>> RFC also because of lack of doc, and:
>>
>> Currently, one can do
>>
>> make -C t t6010-merge-base.sh
>>
>> or even
>>
>> make -C t t601*
>>
>> which is cool but undocumented. If we want this with prove it requires more
>> effort, or shoving prove into the $(T) target rule which is doable, of course.
>> I'm just wondering whether it's accepted to introduce a specific rule for prove
>> at all.
> 
> I like the rationale behind this, but fwiw. I already had a patch that
> was ejected for this:
> 
>     http://article.gmane.org/gmane.comp.version-control.git/146566
> 
> Quoth Junio:
> 
>     "Besides, "make -j15 test" from the toplevel already runs the
>     tests in parallel.  I don't see much point in this change."
>

Sorry for the kind of dupe then. Though I can understand that the fixed options back then didn't find Junio's approval. Maybe it's different now, when it's completely interchangeable with make test?

Show 7 quoted lines
> But I'd like to have it so that I could `make && make test_harness`
> and get readable test output.
> 
> But we should probably be using t/harness to run them, not
> prove(1). Then we would make the test_harness target respect the
> HARNESS_OPTIONS variable, which I e.g. have set to HARNESS_OPTIONS=j9
> on my system.

Didn't know about that one either. What does t/harness differently/better from/than prove? Does it take the same options? Is t/harness respecting the SHELL_PATH for the tests? Running a test under make test and make prove/harness needs to be completely equivalent.

Michael
Ævar Arnfjörð Bjarmason· Oct 14, 2010, 09:33 UTC · re: Michael J Gruber · lore

Re: [RFC PATCH] Makefile: new prove target for running the tests with TAP

On Thu, Oct 14, 2010 at 09:21, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 40 quoted lines
> Ævar Arnfjörð Bjarmason venit, vidit, dixit 14.10.2010 11:05:
>> On Thu, Oct 14, 2010 at 08:53, Michael J Gruber
>> <git@drmicha.warpmail.net> wrote:
>>> Introduce a new make target "prove" which runs the tests via "prove" (or
>>> $(PROVE) if set) so that one does not have to cd around any more. One
>>> can simply use "make prove" for "make test" or go wild as in:
>>>
>>> GIT_SKIP_TESTS='t[0-4]??? t91?? t9200.8' GIT_PROVE_OPTS="-j9 -v" GIT_TEST_OPTS="--verbose --debug" make prove
>>>
>>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>>> ---
>>> RFC also because of lack of doc, and:
>>>
>>> Currently, one can do
>>>
>>> make -C t t6010-merge-base.sh
>>>
>>> or even
>>>
>>> make -C t t601*
>>>
>>> which is cool but undocumented. If we want this with prove it requires more
>>> effort, or shoving prove into the $(T) target rule which is doable, of course.
>>> I'm just wondering whether it's accepted to introduce a specific rule for prove
>>> at all.
>>
>> I like the rationale behind this, but fwiw. I already had a patch that
>> was ejected for this:
>>
>>     http://article.gmane.org/gmane.comp.version-control.git/146566
>>
>> Quoth Junio:
>>
>>     "Besides, "make -j15 test" from the toplevel already runs the
>>     tests in parallel.  I don't see much point in this change."
>>
>
> Sorry for the kind of dupe then. Though I can understand that the fixed
> options back then didn't find Junio's approval. Maybe it's different
> now, when it's completely interchangeable with make test?
Maybe. I try not to decipher him too much :)

But a patch for this that'd be acceptable would be great. Good that you're working on it.

Show 12 quoted lines
>> But I'd like to have it so that I could `make && make test_harness`
>> and get readable test output.
>>
>> But we should probably be using t/harness to run them, not
>> prove(1). Then we would make the test_harness target respect the
>> HARNESS_OPTIONS variable, which I e.g. have set to HARNESS_OPTIONS=j9
>> on my system.
>
> Didn't know about that one either. What does t/harness
> differently/better from/than prove? Does it take the same options? Is
> t/harness respecting the SHELL_PATH for the tests? Running a test under
> make test and make prove/harness needs to be completely equivalent.

t/harness is just using the library that prove(1) uses directly. We already use it for "make smoke". The advantage is that you can run custom setup code there, e.g. this is from perl.git's own t/harness:

    my $jobs = $ENV{TEST_JOBS};
    my ($rules, $state, $color);
    if ($ENV{HARNESS_OPTIONS}) {
        for my $opt ( split /:/, $ENV{HARNESS_OPTIONS} ) {
            if ( $opt =~ /^j(\d*)$/ ) {
                $jobs ||= $1 || 9;
            }
            elsif ( $opt eq 'c' ) {
                $color = 1;
            }
            else {
                die "Unknown HARNESS_OPTIONS item: $opt\n";
            }
        }
    }

So if we expanded t/harness to use that and added a "test_harness" target we could make it grok the TEST_JOBS and HARNESS_OPTIONS environment variables.

The output would be the same (see "make smoke"), and we can make it respect SHELL_PATH.

Jonathan Nieder· Oct 14, 2010, 16:27 UTC · re: Michael J Gruber · lore

Re: [RFC PATCH] Makefile: new prove target for running the tests with TAP

Michael J Gruber wrote:
> Introduce a new make target "prove" which runs the tests via "prove"
I like it. :)
I've been using
	prove -j2 --shuffle t[0-9]*.sh

and would be happier to be able to dump GIT_PROVE_OPTS="-j2 --shuffle" in my config.mak and just run "make prove" instead of that unwieldly glob.

Show 12 quoted lines
> RFC also because of lack of doc, and:
> 
> Currently, one can do
> 
> make -C t t6010-merge-base.sh
> 
> or even
> 
> make -C t t601*
> 
> which is cool but undocumented. If we want this with prove it requires more
> effort
Doesn't "prove t601*" work well for that use case already?
> --- a/t/Makefile
> +++ b/t/Makefile
[...]
Show 6 quoted lines
> +prove: pre-clean
> +	@echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
> +	$(MAKE) clean
> +
>  $(T):
>  	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)

Not about this patch, but is that GIT_CONFIG setting needed? I would have guessed rather that test-lib.sh would unset GIT_CONFIG as soon as possible.

Jeff King· Oct 14, 2010, 17:23 UTC · re: Michael J Gruber · lore

Re: [RFC PATCH] Makefile: new prove target for running the tests with TAP

On Thu, Oct 14, 2010 at 10:53:36AM +0200, Michael J Gruber wrote:
> +prove: pre-clean
> +	@echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
> +	$(MAKE) clean
> +

I like the idea. I think the prove output is a little nicer for things like -j16, but it's a pain to construct the command line (especially as I use --root in GIT_TEST_OPTS to get a significant speedup).

I wonder if anybody is really interested in switching between the "prove" and regular test targets, though. I would think you either like to use prove or not. So perhaps it makes more sense to put a configurable switch for the default target, like the patch below. Then you can just "make test" (or "make" in the test directory) as usual.

Show changes to t/Makefile +9 −1
diff --git a/t/Makefile b/t/Makefile
index c7baefb..2b4d4ac 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -11,6 +11,8 @@ SHELL_PATH ?= $(SHELL)
 PERL_PATH ?= /usr/bin/perl
 TAR ?= $(TAR)
 RM ?= rm -f
+PROVE ?= prove
+DEFAULT_TEST_TARGET ?= test
 
 # Shell quote;
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
@@ -18,9 +20,15 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 T = $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)
 TSVN = $(wildcard t91[0-9][0-9]-*.sh)
 
-all: pre-clean
+all: $(DEFAULT_TEST_TARGET)
+
+test: pre-clean
 	$(MAKE) aggregate-results-and-cleanup
 
+prove: pre-clean
+	@echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
+	$(MAKE) clean
+
 $(T):
 	@echo "*** $@ ***"; GIT_CONFIG=.git/config '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
Junio C Hamano· Oct 27, 2010, 00:18 UTC · re: Jeff King · lore

Re: [RFC PATCH] Makefile: new prove target for running the tests with TAP

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> On Thu, Oct 14, 2010 at 10:53:36AM +0200, Michael J Gruber wrote:
>
>> +prove: pre-clean
>> +	@echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
>> +	$(MAKE) clean
>> +
>
> I like the idea. I think the prove output is a little nicer for things
> like -j16, but it's a pain to construct the command line (especially as
> I use --root in GIT_TEST_OPTS to get a significant speedup).
>
> I wonder if anybody is really interested in switching between the
> "prove" and regular test targets, though. I would think you either like
> to use prove or not. So perhaps it makes more sense to put a
> configurable switch for the default target, like the patch below. Then
> you can just "make test" (or "make" in the test directory) as usual.
I kind of like this.  Perhaps with something like this squashed in?

-- >8 -- test: allow "prove" to be used as the main test harness

The prove output is a little easier to read for things like -j16.
---
 t/README |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Show changes to t/README +6 −0
diff --git a/t/README b/t/README
index a1eb7c8..c548bf1 100644
--- a/t/README
+++ b/t/README
@@ -50,6 +50,12 @@ prove and other harnesses come with a lot of useful options. The
     # Repeat until no more failures
     $ prove -j 15 --state=failed,save ./t[0-9]*.sh
 
+You can give DEFAULT_TEST_TARGET=prove on the make command (or define it
+in config.mak) to cause "make test" to run tests under prove.
+GIT_PROVE_OPTS can be used to pass additional options, e.g.
+
+    $ make DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS='--timer --jobs 16' test
+
 You can also run each test individually from command line, like this:
 
     $ sh ./t3010-ls-files-killed-modified.sh
Jeff King· Oct 27, 2010, 04:57 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] Makefile: new prove target for running the tests with TAP

On Tue, Oct 26, 2010 at 05:18:22PM -0700, Junio C Hamano wrote:
> > like -j16, but it's a pain to construct the command line (especially as
> > I use --root in GIT_TEST_OPTS to get a significant speedup).
> [..]
> I kind of like this.  Perhaps with something like this squashed in?

Yeah, looks good to me. I guess you can steal the commit message and authorship from Michael's original.

Ævar mentioned something about t/harness, which I honestly have no idea about (I don't have the right perl modules installed to run it). But maybe that could be an alternate DEFAULT_TEST_TARGET. I dunno.

-Peff
Ævar Arnfjörð Bjarmason· Oct 27, 2010, 11:00 UTC · re: Jeff King · lore

Re: [RFC PATCH] Makefile: new prove target for running the tests with TAP

On Wed, Oct 27, 2010 at 06:57, Jeff King <peff@peff.net> wrote:
Show 13 quoted lines
> On Tue, Oct 26, 2010 at 05:18:22PM -0700, Junio C Hamano wrote:
>
>> > like -j16, but it's a pain to construct the command line (especially as
>> > I use --root in GIT_TEST_OPTS to get a significant speedup).
>> [..]
>> I kind of like this.  Perhaps with something like this squashed in?
>
> Yeah, looks good to me. I guess you can steal the commit message and
> authorship from Michael's original.
>
> Ævar mentioned something about t/harness, which I honestly have no idea
> about (I don't have the right perl modules installed to run it). But
> maybe that could be an alternate DEFAULT_TEST_TARGET. I dunno.
The non-standard module there is only needed for the smoke testing.

If we used t/harness for running "make test" that code could be modified to run anywhere prove itself does.

Anyway, if there's a patch now to invoke prove(1) let's use that. We can change it to t/harness later if that makes sense.

Michael J Gruber· Oct 29, 2010, 13:19 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC PATCH] Makefile: new prove target for running the tests with TAP

Ævar Arnfjörð Bjarmason venit, vidit, dixit 27.10.2010 13:00:
Show 22 quoted lines
> On Wed, Oct 27, 2010 at 06:57, Jeff King <peff@peff.net> wrote:
>> On Tue, Oct 26, 2010 at 05:18:22PM -0700, Junio C Hamano wrote:
>>
>>>> like -j16, but it's a pain to construct the command line (especially as
>>>> I use --root in GIT_TEST_OPTS to get a significant speedup).
>>> [..]
>>> I kind of like this.  Perhaps with something like this squashed in?
>>
>> Yeah, looks good to me. I guess you can steal the commit message and
>> authorship from Michael's original.
>>
>> Ævar mentioned something about t/harness, which I honestly have no idea
>> about (I don't have the right perl modules installed to run it). But
>> maybe that could be an alternate DEFAULT_TEST_TARGET. I dunno.
> 
> The non-standard module there is only needed for the smoke testing.
> 
> If we used t/harness for running "make test" that code could be
> modified to run anywhere prove itself does.
> 
> Anyway, if there's a patch now to invoke prove(1) let's use that. We
> can change it to t/harness later if that makes sense.

Thanks, Junio, for taking this up (and others for discussing it), I've been out of the loop a bit. I'll try and keep up with my other half-baked patches ;)

Michael

← back to recent threads