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

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

[Michael J Gruber]

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

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

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

[Ævar Arnfjörð Bjarmason]

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

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.

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

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

[Michael J Gruber]

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

> 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

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

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 14, 2010 at 09:21, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Æ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.

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

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

[Jonathan Nieder]

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.

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

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

[Jeff King]

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.

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)
 

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

[Jeff King]

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

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Michael J Gruber]

Ævar Arnfjörð Bjarmason venit, vidit, dixit 27.10.2010 13:00:
> 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