[PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Ilya Bobyr]

We used to show "(missing )" next to tests skipped because they are
specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.
---
 t/test-lib.sh |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..89a405b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -446,25 +446,28 @@ test_finish_ () {
 
 test_skip () {
 	to_skip=
+	skipped_reason=
 	if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
 	then
 		to_skip=t
+		skipped_reason="matched GIT_SKIP_TESTS"
 	fi
 	if test -z "$to_skip" && test -n "$test_prereq" &&
 	   ! test_have_prereq "$test_prereq"
 	then
 		to_skip=t
-	fi
-	case "$to_skip" in
-	t)
+
 		of_prereq=
 		if test "$missing_prereq" != "$test_prereq"
 		then
 			of_prereq=" of $test_prereq"
 		fi
-
+		skipped_reason="missing $missing_prereq${of_prereq}"
+	fi
+	case "$to_skip" in
+	t)
 		say_color skip >&3 "skipping test: $@"
-		say_color skip "ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
+		say_color skip "ok $test_count # skip $1 ($skipped_reason)"
 		: true
 		;;
 	*)
-- 
1.7.9

[PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Ilya Bobyr]

This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when debugging.
---
 t/README      |   15 +++++++++++++++
 t/test-lib.sh |    8 ++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index caeeb9d..f939987 100644
--- a/t/README
+++ b/t/README
@@ -187,6 +187,21 @@ and either can match the "t[0-9]{4}" part to skip the whole
 test, or t[0-9]{4} followed by ".$number" to say which
 particular test to skip.
 
+Sometimes the opposite is desired - ability to execute only one or
+several tests.  Mostly while debugging tests.  For that you can say
+
+    $ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
+
+or, similrary to GIT_SKIP_TESTS
+
+    $ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
+
+In additiona to matching against "<test suite number>.<test number>"
+GIT_TEST_ONLY is matched against just the test numbes.  This comes
+handy when you are running only one test:
+
+    $ GIT_TEST_ONLY='[0-8]' sh ./t9200-git-cvsexport-commit.sh
+
 Note that some tests in the existing test suite rely on previous
 test item, so you cannot arbitrarily disable one and expect the
 remainder of test to check what the test originally was intended
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 89a405b..12bf436 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -464,6 +464,14 @@ test_skip () {
 		fi
 		skipped_reason="missing $missing_prereq${of_prereq}"
 	fi
+	if test -z "$to_skip" && test -n "$GIT_TEST_ONLY" &&
+		! match_pattern_list $this_test.$test_count $GIT_TEST_ONLY &&
+		! match_pattern_list $test_count $GIT_TEST_ONLY
+	then
+		to_skip=t
+		skipped_reason="not in GIT_TEST_ONLY"
+	fi
+
 	case "$to_skip" in
 	t)
 		say_color skip >&3 "skipping test: $@"
-- 
1.7.9

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Philip Oakley]

Minor nits.

From: "Ilya Bobyr" <ilya.bobyr@gmail.com>
> This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when 
> debugging.
> ---
> t/README      |   15 +++++++++++++++
> t/test-lib.sh |    8 ++++++++
> 2 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/t/README b/t/README
> index caeeb9d..f939987 100644
> --- a/t/README
> +++ b/t/README
> @@ -187,6 +187,21 @@ and either can match the "t[0-9]{4}" part to skip 
> the whole
> test, or t[0-9]{4} followed by ".$number" to say which
> particular test to skip.
>
> +Sometimes the opposite is desired - ability to execute only one or
> +several tests.  Mostly while debugging tests.  For that you can say
> +
> +    $ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
> +
> +or, similrary to GIT_SKIP_TESTS
> +
> +    $ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
> +
> +In additiona to matching against "<test suite number>.<test number>"
> +GIT_TEST_ONLY is matched against just the test .  This comes
s/numbes/numbers/

> +handy when you are running only one test:
s/handy/in handy/

> +
> +    $ GIT_TEST_ONLY='[0-8]' sh ./t9200-git-cvsexport-commit.sh
> +
> Note that some tests in the existing test suite rely on previous
> test item, so you cannot arbitrarily disable one and expect the
> remainder of test to check what the test originally was intended
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 89a405b..12bf436 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -464,6 +464,14 @@ test_skip () {
>  fi
>  skipped_reason="missing $missing_prereq${of_prereq}"
>  fi
> + if test -z "$to_skip" && test -n "$GIT_TEST_ONLY" &&
> + ! match_pattern_list $this_test.$test_count $GIT_TEST_ONLY &&
> + ! match_pattern_list $test_count $GIT_TEST_ONLY
> + then
> + to_skip=t
> + skipped_reason="not in GIT_TEST_ONLY"
> + fi
> +
>  case "$to_skip" in
>  t)
>  say_color skip >&3 "skipping test: $@"
> -- 
Philip 

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Eric Sunshine]

On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
> This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when debugging.

To be grammatically similar to GIT_SKIP_TESTS, perhaps name it GIT_RUN_TESTS?

> ---
>  t/README      |   15 +++++++++++++++
>  t/test-lib.sh |    8 ++++++++
>  2 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/t/README b/t/README
> index caeeb9d..f939987 100644
> --- a/t/README
> +++ b/t/README
> @@ -187,6 +187,21 @@ and either can match the "t[0-9]{4}" part to skip the whole
>  test, or t[0-9]{4} followed by ".$number" to say which
>  particular test to skip.
>
> +Sometimes the opposite is desired - ability to execute only one or
> +several tests.  Mostly while debugging tests.  For that you can say
> +
> +    $ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
> +
> +or, similrary to GIT_SKIP_TESTS
> +
> +    $ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
> +
> +In additiona to matching against "<test suite number>.<test number>"

s/additiona/addition/

Plus the other typos already mentioned by Philip...

> +GIT_TEST_ONLY is matched against just the test numbes.  This comes
> +handy when you are running only one test:
> +
> +    $ GIT_TEST_ONLY='[0-8]' sh ./t9200-git-cvsexport-commit.sh
> +
>  Note that some tests in the existing test suite rely on previous
>  test item, so you cannot arbitrarily disable one and expect the
>  remainder of test to check what the test originally was intended
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 89a405b..12bf436 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -464,6 +464,14 @@ test_skip () {
>                 fi
>                 skipped_reason="missing $missing_prereq${of_prereq}"
>         fi
> +       if test -z "$to_skip" && test -n "$GIT_TEST_ONLY" &&
> +               ! match_pattern_list $this_test.$test_count $GIT_TEST_ONLY &&
> +               ! match_pattern_list $test_count $GIT_TEST_ONLY
> +       then
> +               to_skip=t
> +               skipped_reason="not in GIT_TEST_ONLY"
> +       fi
> +
>         case "$to_skip" in
>         t)
>                 say_color skip >&3 "skipping test: $@"
> --
> 1.7.9
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Ilya Bobyr]

On 3/3/2014 3:03 PM, Eric Sunshine wrote:
> On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
>> This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when debugging.
> To be grammatically similar to GIT_SKIP_TESTS, perhaps name it GIT_RUN_TESTS?

There is actually an upside in the fact that the name is "different 
enough".  When you pull a command from a history it is easier to see if 
it is the excluding or the including one.
Maybe we can have a third opinion here?

>> ---
>>   t/README      |   15 +++++++++++++++
>>   t/test-lib.sh |    8 ++++++++
>>   2 files changed, 23 insertions(+), 0 deletions(-)
>>
>> diff --git a/t/README b/t/README
>> index caeeb9d..f939987 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -187,6 +187,21 @@ and either can match the "t[0-9]{4}" part to skip the whole
>>   test, or t[0-9]{4} followed by ".$number" to say which
>>   particular test to skip.
>>
>> +Sometimes the opposite is desired - ability to execute only one or
>> +several tests.  Mostly while debugging tests.  For that you can say
>> +
>> +    $ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
>> +
>> +or, similrary to GIT_SKIP_TESTS
>> +
>> +    $ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
>> +
>> +In additiona to matching against "<test suite number>.<test number>"
> s/additiona/addition/
>
> Plus the other typos already mentioned by Philip...

Thank you.  I will include all of those in the next version of the patch.

> [...]

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
>> This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when debugging.
>
> To be grammatically similar to GIT_SKIP_TESTS, perhaps name it GIT_RUN_TESTS?

I actually do not like the interface to use two variables very much.
Can't we just allow negative entries on "to be skipped" list?

That is

	GIT_SKIP_TESTS='t9??? !t91??'

would skip nine-thousand series, but would run 91xx series, and all
the others are not excluded.

Simple rules to consider:

 - If the list consists of _only_ negated patterns, pretend that
   there is "unless otherwise specified with negatives, skip all
   tests", i.e. treat GIT_SKIP_TESTS='!t91??' just the same way you
   would treat GIT_SKIP_TESTS='* !t91??'.

 - The orders should not matter for simplicity of the semantics;
   before running each test, check if it matches any negative (and
   run it if it matches, without looking at any positives), and
   otherwise check if it matches any positive (and skip it if it
   does not).

Hmm?

>> ---
>>  t/README      |   15 +++++++++++++++
>>  t/test-lib.sh |    8 ++++++++
>>  2 files changed, 23 insertions(+), 0 deletions(-)
>>
>> diff --git a/t/README b/t/README
>> index caeeb9d..f939987 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -187,6 +187,21 @@ and either can match the "t[0-9]{4}" part to skip the whole
>>  test, or t[0-9]{4} followed by ".$number" to say which
>>  particular test to skip.
>>
>> +Sometimes the opposite is desired - ability to execute only one or
>> +several tests.  Mostly while debugging tests.  For that you can say
>> +
>> +    $ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
>> +
>> +or, similrary to GIT_SKIP_TESTS
>> +
>> +    $ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
>> +
>> +In additiona to matching against "<test suite number>.<test number>"
>
> s/additiona/addition/
>
> Plus the other typos already mentioned by Philip...
>
>> +GIT_TEST_ONLY is matched against just the test numbes.  This comes
>> +handy when you are running only one test:
>> +
>> +    $ GIT_TEST_ONLY='[0-8]' sh ./t9200-git-cvsexport-commit.sh
>> +
>>  Note that some tests in the existing test suite rely on previous
>>  test item, so you cannot arbitrarily disable one and expect the
>>  remainder of test to check what the test originally was intended
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 89a405b..12bf436 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -464,6 +464,14 @@ test_skip () {
>>                 fi
>>                 skipped_reason="missing $missing_prereq${of_prereq}"
>>         fi
>> +       if test -z "$to_skip" && test -n "$GIT_TEST_ONLY" &&
>> +               ! match_pattern_list $this_test.$test_count $GIT_TEST_ONLY &&
>> +               ! match_pattern_list $test_count $GIT_TEST_ONLY
>> +       then
>> +               to_skip=t
>> +               skipped_reason="not in GIT_TEST_ONLY"
>> +       fi
>> +
>>         case "$to_skip" in
>>         t)
>>                 say_color skip >&3 "skipping test: $@"
>> --
>> 1.7.9
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Ilya Bobyr]

On 3/3/2014 3:26 PM, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
>>> This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when debugging.
>> To be grammatically similar to GIT_SKIP_TESTS, perhaps name it GIT_RUN_TESTS?
> I actually do not like the interface to use two variables very much.
> Can't we just allow negative entries on "to be skipped" list?
>
> That is
>
> 	GIT_SKIP_TESTS='t9??? !t91??'
>
> would skip nine-thousand series, but would run 91xx series, and all
> the others are not excluded.
>
> Simple rules to consider:
>
>   - If the list consists of _only_ negated patterns, pretend that
>     there is "unless otherwise specified with negatives, skip all
>     tests", i.e. treat GIT_SKIP_TESTS='!t91??' just the same way you
>     would treat GIT_SKIP_TESTS='* !t91??'.
>
>   - The orders should not matter for simplicity of the semantics;
>     before running each test, check if it matches any negative (and
>     run it if it matches, without looking at any positives), and
>     otherwise check if it matches any positive (and skip it if it
>     does not).
>
> Hmm?

I can do that.  But I am not sure that matches the use cases I had in 
mind the best.

First use case is that while developing I want to run tests frequently 
and I have a specific test that I am working on at the moment.
That test is broken and I am trying to fix it (TDD).
I want to run just the initialization test(s) and then that specific test.
Running everything is quite slow.

GIT_RUN_ONLY addresses the TDD case.

Second case is when I broke one or more tests and want to figure out 
what is wrong.
In this case running tests after the broken one will clutter the output 
directory and will make debugging somewhat harder, especially if I am 
not familiar with all the tests.

For the second case I was actually thinking that something like 
"<t9100.32" would be useful, where 32 is the broken test.

Maybe we can come up with an interface that covers all 3 cases?

While exclusion can be used it adds an extra step to both cases, as you 
need to mentally negate what you want first.

It might be that we are looking at different use cases, as you are 
talking about whole test suits.

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Junio C Hamano]

Ilya Bobyr <ilya.bobir@gmail.com> writes:

> It might be that we are looking at different use cases, as you are
> talking about whole test suits.

I do not think so.

I do not see anything prevents you from saying

	GIT_SKIP_TESTS='t0000 !t0000.1 !t0000.4'

to specify test-pieces in individual tests so that you can run the
setup step (step .1) and the specific test (step .4) without running
two tests in between.

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Ilya Bobyr]

On 3/3/2014 4:08 PM, Junio C Hamano wrote:
> Ilya Bobyr <ilya.bobir@gmail.com> writes:
>
>> It might be that we are looking at different use cases, as you are
>> talking about whole test suits.
> I do not think so.

Good :)
I am trying to understand the use cases.  And make sure we are talking 
about the same ones.
I am not sure what are the use cases for GIT_SKIP_TESTS.

I think that while in it really nice when an interface allows to do new 
things, the main use case (or use cases) should be as easy and obvious 
in the first place.
If the target is the TDD use case I described, then it appears that a 
user needs to do double negation.  That is what concerns me.

> I do not see anything prevents you from saying
>
> 	GIT_SKIP_TESTS='t0000 !t0000.1 !t0000.4'
>
> to specify test-pieces in individual tests so that you can run the
> setup step (step .1) and the specific test (step .4) without running
> two tests in between.

While it could be done, it looks less obvious than this:

     GIT_TEST_ONLY='1 4' ./t0001-init.sh

What if we do what you proposed, but with GIT_RUN_TESTS?
If we want to have one interface, maybe, building on top of a "negation" 
(GIT_SKIP_TESTS) is not very good.
At the same time, GIT_TEST_ONLY is also too specific for a generic 
interface.
I could add GIT_RUN_TESTS and allow it to have all of the features, thus 
making GIT_SKIP_TESTS a working but deprecated tool.

Running specific tests:
     GIT_RUN_TESTS='t0000'
     GIT_RUN_TESTS='t0000.1 t0002'
     GIT_RUN_TESTS='1 3 7'

Negating some tests:
     GIT_RUN_TESTS='!t0000'
     GIT_RUN_TESTS='!t0000.1'
     GIT_RUN_TESTS='!1 !3 !7'

The above would work exactly as you described but with one less level of 
negation.  Default is everything, unless at least one positive pattern 
is given.

Later on range specification could added:
     GIT_RUN_TESTS='<11'

At least for now that would cover most use cases that I can think of 
that look reasonable.

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Junio C Hamano]

Ilya Bobyr <ilya.bobir@gmail.com> writes:

> While it could be done, it looks less obvious than this:
>
>     GIT_TEST_ONLY='1 4' ./t0001-init.sh

If you are thinking about affecting only one test, then you
shouldn't be mucking with environment variables in the first place,
primarily because running:

    $ GIT_TEST_ONLY='1 4' make test

to run test .1 and .4 of all the test scripts would not make any
sense.  I think your "simplicity" argument is a total red-herring.
Of course if you do not have to say the test script name, your
specification would be shorter, but that is only because your
specification is not specific enough to be useful.

Giving that as a command line argument to the specific script, e.g.

    $ sh ./t0001-init.sh --only='1 4'

might make some sense, but the above GIT_TEST_ONLY does not make any
sense from the UI point of view.

There are many reasons that makes me unenthused about this line of
change in the first place:

 * Both at the philosophical level and at the practical level, I've
   found that it always makes sense to run most of the tests, i.e.
   skipping ought to be an exception not the norm. Over the course
   of this project, I often saw an alleged fix to one part of the
   system introduces breakages that are caught by tests that checks
   parts of the system that does not have any superficial link to it
   (e.g. update the refs code and find a rebase test break).

 * Even though GIT_SKIP_TESTS mechanism still allows you to skip
   individual test pieces, it has never been a serious "feature" in
   the first place. Many of the tests unfortunately do rely on state
   previous sequences of tests left behind, so it is not realistic
   to expect that you can skip test pieces randomly and exercise
   later test pieces reliably.

 * The numbering of individual test pieces can easily change by new
   tests inserted in the middle; again, many tests do take advantge
   of the states earlier tests leave behind, so "do not add new
   tests in the middle" is not a realistic rule to enforce, unless
   you are willing to clean up existing test scripts so that each
   test piece is independent from all the previous ones.

The latter two makes the ability to skip individual test pieces a
"theoretically it could be made useful but practically not so much"
misfeature.  I am very hesitant to see the test framework code
churned only to enhance its "usefulness" when there isn't any in the
first place, without first making changes that fundamentally
improves its usefulness (e.g. to solve "test numbering is not
stable" problem, you could identify the tests with test names
instead of numbers to make it more stable, but that is not what your
patch is even attempting to do).

I view such a change as merely a code churn, damaging the codebase
that is already less nice than ideal and turning it more difficult
to fix properly to make it truly nicer later.

My suggestion to cram everything into GIT_SKIP_TESTS is primarily
because it is one way I can easily see how it allows us to limit the
"damage" to the codebase--the suggested change could be contained
within a single shell function "match_pattern_list" and no other
code has to change to support it.  I am not saying it is the only
way, but glancing at your patch, adding an extra environment
variable would need to also modify its callers, so the extent of the
damage to the codebase seemed somewhat larger.

So, I dunno.  I am not yet enthused.

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Ilya Bobyr]

On 3/4/2014 12:29 AM, Junio C Hamano wrote:
> Ilya Bobyr <ilya.bobir@gmail.com> writes:
>
>> While it could be done, it looks less obvious than this:
>>
>>      GIT_TEST_ONLY='1 4' ./t0001-init.sh
> If you are thinking about affecting only one test,

Yes, that is the use case: when I am developing a specific feature I 
want to run just one test for that feature over and over, while I am 
working on that specific thing.
Not the whole test suite (like "t0001"), but just the new case that I've 
added to the end, for example.  Plus one or more tests that setup enough 
environment for it.

> then you
> shouldn't be mucking with environment variables in the first place,
> primarily because running:
>
>      $ GIT_TEST_ONLY='1 4' make test
>
> to run test .1 and .4 of all the test scripts would not make any
> sense.

No it does not.  It only makes sense for one test suite.

> I think your "simplicity" argument is a total red-herring.
> Of course if you do not have to say the test script name, your
> specification would be shorter, but that is only because your
> specification is not specific enough to be useful.

In my case it is very useful :)
This is why I am saying that we might be talking about different cases: 
you are talking about the test suite level, while the issue I am trying 
to address an issue at an individual test level.

> Giving that as a command line argument to the specific script, e.g.
>
>      $ sh ./t0001-init.sh --only='1 4'
>
> might make some sense, but the above GIT_TEST_ONLY does not make any
> sense from the UI point of view.

No problem, I guess I can make it look like that - with '--only'.
Maybe '--tests'?  Then the same negation syntax could be used as 
previously discussed.
As well as range syntax.

> There are many reasons that makes me unenthused about this line of
> change in the first place:
>
>   * Both at the philosophical level and at the practical level, I've
>     found that it always makes sense to run most of the tests, i.e.
>     skipping ought to be an exception not the norm. Over the course
>     of this project, I often saw an alleged fix to one part of the
>     system introduces breakages that are caught by tests that checks
>     parts of the system that does not have any superficial link to it
>     (e.g. update the refs code and find a rebase test break).

My main argument is the time.  When testing Git as a whole or a feature 
as a whole there is no reason to skip some tests.
When working on a specific piece I may run the same test 100 times easily.
Here is what I see on my Cygwin:

     $ time ./t0001-init.sh
     [...]
     1..36

     real    0m6.693s
     user    0m1.505s
     sys     0m3.937s


     $ time GIT_SKIP_TESTS='t0001.[36789] t0001.??' ./t0001-init.sh
     [...]
     1..36

     real    0m3.313s
     user    0m0.769s
     sys     0m1.844s

So skipping 34 tests that I am not interested in save a bit more that 
50% of the time.
While it would be really nice if it would be faster, this speedup is a 
pretty simple one.

>   * Even though GIT_SKIP_TESTS mechanism still allows you to skip
>     individual test pieces, it has never been a serious "feature" in
>     the first place. Many of the tests unfortunately do rely on state
>     previous sequences of tests left behind, so it is not realistic
>     to expect that you can skip test pieces randomly and exercise
>     later test pieces reliably.
>
>   * The numbering of individual test pieces can easily change by new
>     tests inserted in the middle; again, many tests do take advantge
>     of the states earlier tests leave behind, so "do not add new
>     tests in the middle" is not a realistic rule to enforce, unless
>     you are willing to clean up existing test scripts so that each
>     test piece is independent from all the previous ones.

Both are true, but do not apply to the TDD case.
Neither they apply to a case when a test is broken and I want to execute 
everything up to that test.

> The latter two makes the ability to skip individual test pieces a
> "theoretically it could be made useful but practically not so much"
> misfeature.  I am very hesitant to see the test framework code
> churned only to enhance its "usefulness" when there isn't any in the
> first place, without first making changes that fundamentally
> improves its usefulness (e.g. to solve "test numbering is not
> stable" problem, you could identify the tests with test names
> instead of numbers to make it more stable, but that is not what your
> patch is even attempting to do).

If you see a way to address my problems, I might be able to code it the 
way you want it to be.

> [...]

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Junio C Hamano]

Ilya Bobyr <ilya.bobir@gmail.com> writes:

> On 3/4/2014 12:29 AM, Junio C Hamano wrote:
> ...
>> then you
>> shouldn't be mucking with environment variables in the first place,
>> primarily because running:
>>
>>      $ GIT_TEST_ONLY='1 4' make test
>>
>> to run test .1 and .4 of all the test scripts would not make any
>> sense.
>
> No it does not.  It only makes sense for one test suite.
>
>> I think your "simplicity" argument is a total red-herring.
>> Of course if you do not have to say the test script name, your
>> specification would be shorter, but that is only because your
>> specification is not specific enough to be useful.
>
> In my case it is very useful :)

It invites a nonsense usage (i.e. running "make test" under that
environment variable setting); that is not a good trade-off.

>>   * Even though GIT_SKIP_TESTS mechanism still allows you to skip
>>     individual test pieces, it has never been a serious "feature" in
>>     the first place. Many of the tests unfortunately do rely on state
>>     previous sequences of tests left behind, so it is not realistic
>>     to expect that you can skip test pieces randomly and exercise
>>     later test pieces reliably.
>>
>>   * The numbering of individual test pieces can easily change by new
>>     tests inserted in the middle; again, many tests do take advantge
>>     of the states earlier tests leave behind, so "do not add new
>>     tests in the middle" is not a realistic rule to enforce, unless
>>     you are willing to clean up existing test scripts so that each
>>     test piece is independent from all the previous ones.
>
> Both are true, but do not apply to the TDD case.

The existing tests are designed to be black-box tests, not function
level unit tests, and touching lower level code carelessly affects
other parts of the system you did not know the interactions about.

What does "TDD case" change anything in that equation?

Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Philip Oakley]

From: "Ilya Bobyr" <ilya.bobyr@gmail.com>
> We used to show "(missing )" next to tests skipped because they are
> specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" 
> instead.

The message below forgets the "by". Otherwise looks sensible.

> ---
> t/test-lib.sh |   13 ++++++++-----
> 1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1531c24..89a405b 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -446,25 +446,28 @@ test_finish_ () {
>
> test_skip () {
>  to_skip=
> + skipped_reason=
>  if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
>  then
>  to_skip=t
> + skipped_reason="matched GIT_SKIP_TESTS"

s/matched GIT_SKIP_TESTS/matched by GIT_SKIP_TESTS/

>  fi
>  if test -z "$to_skip" && test -n "$test_prereq" &&
>     ! test_have_prereq "$test_prereq"
>  then
>  to_skip=t
> - fi
> - case "$to_skip" in
> - t)
> +
>  of_prereq=
>  if test "$missing_prereq" != "$test_prereq"
>  then
>  of_prereq=" of $test_prereq"
>  fi
> -
> + skipped_reason="missing $missing_prereq${of_prereq}"
> + fi
> + case "$to_skip" in
> + t)
>  say_color skip >&3 "skipping test: $@"
> - say_color skip "ok $test_count # skip $1 (missing 
> $missing_prereq${of_prereq})"
> + say_color skip "ok $test_count # skip $1 ($skipped_reason)"
>  : true
>  ;;
>  *)
> -- 
> 1.7.9

Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Ilya Bobyr]

On 3/3/2014 7:11 AM, Philip Oakley wrote:
> From: "Ilya Bobyr" <ilya.bobyr@gmail.com>
>> We used to show "(missing )" next to tests skipped because they are
>> specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.
>
> The message below forgets the "by".

I'll fix the commit message.  I think the output is long enough, while 
"by" does not add any information.

> Otherwise looks sensible.

Thanks for looking at it :)

>
>> ---
>> t/test-lib.sh |   13 ++++++++-----
>> 1 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 1531c24..89a405b 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -446,25 +446,28 @@ test_finish_ () {
>>
>> test_skip () {
>>  to_skip=
>> + skipped_reason=
>>  if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
>>  then
>>  to_skip=t
>> + skipped_reason="matched GIT_SKIP_TESTS"
>
> s/matched GIT_SKIP_TESTS/matched by GIT_SKIP_TESTS/
>
>>  [...]

Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Eric Sunshine]

On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
> We used to show "(missing )" next to tests skipped because they are
> specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.

Bikeshedding: That's pretty verbose. Perhaps just say "(excluded)"?

> ---
>  t/test-lib.sh |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1531c24..89a405b 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -446,25 +446,28 @@ test_finish_ () {
>
>  test_skip () {
>         to_skip=
> +       skipped_reason=
>         if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
>         then
>                 to_skip=t
> +               skipped_reason="matched GIT_SKIP_TESTS"
>         fi
>         if test -z "$to_skip" && test -n "$test_prereq" &&
>            ! test_have_prereq "$test_prereq"
>         then
>                 to_skip=t
> -       fi
> -       case "$to_skip" in
> -       t)
> +
>                 of_prereq=
>                 if test "$missing_prereq" != "$test_prereq"
>                 then
>                         of_prereq=" of $test_prereq"
>                 fi
> -
> +               skipped_reason="missing $missing_prereq${of_prereq}"
> +       fi
> +       case "$to_skip" in
> +       t)
>                 say_color skip >&3 "skipping test: $@"
> -               say_color skip "ok $test_count # skip $1 (missing $missing_prereq${of_prereq})"
> +               say_color skip "ok $test_count # skip $1 ($skipped_reason)"
>                 : true
>                 ;;
>         *)
> --
> 1.7.9
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Ilya Bobyr]

On 3/3/2014 2:59 PM, Eric Sunshine wrote:
> On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
>> We used to show "(missing )" next to tests skipped because they are
>> specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.
> Bikeshedding: That's pretty verbose. Perhaps just say "(excluded)"?

The next patch adds another reason for the test to be skipped, so it 
seems reasonable to say why exactly.
The patch actually makes it say "matched GIT_SKIP_TESTS".
It looks OK on the console.

Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
>> We used to show "(missing )" next to tests skipped because they are
>> specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.
>
> Bikeshedding: That's pretty verbose. Perhaps just say "(excluded)"?

Sounds good, or at least better than matched GIT_SKIP_TESTS, to me ;-).

Thanks.

Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Eric Sunshine]

On Mon, Mar 3, 2014 at 6:12 PM, Ilya Bobyr <ilya.bobir@gmail.com> wrote:
> On 3/3/2014 2:59 PM, Eric Sunshine wrote:
>>
>> On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
>>>
>>> We used to show "(missing )" next to tests skipped because they are
>>> specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.
>>
>> Bikeshedding: That's pretty verbose. Perhaps just say "(excluded)"?
>
>
> The next patch adds another reason for the test to be skipped, so it seems
> reasonable to say why exactly.
> The patch actually makes it say "matched GIT_SKIP_TESTS".
> It looks OK on the console.

Still just bikeshedding:

That new message in patch #2 says "not in GIT_TEST_ONLY", but isn't
"(excluded)" also applicable to that case? Is it important to be able
to distinguish between the two "excluded" reasons?

(No more bikeshedding for me.)

Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> That new message in patch #2 says "not in GIT_TEST_ONLY", but isn't
> "(excluded)" also applicable to that case? Is it important to be able
> to distinguish between the two "excluded" reasons?

An obvious solution is not to *have* two reasons in the first place
;-)

Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Ilya Bobyr]

On 3/3/2014 3:18 PM, Eric Sunshine wrote:
> On Mon, Mar 3, 2014 at 6:12 PM, Ilya Bobyr <ilya.bobir@gmail.com> wrote:
>> On 3/3/2014 2:59 PM, Eric Sunshine wrote:
>>> On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
>>>> We used to show "(missing )" next to tests skipped because they are
>>>> specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.
>>> Bikeshedding: That's pretty verbose. Perhaps just say "(excluded)"?
>>
>> The next patch adds another reason for the test to be skipped, so it seems
>> reasonable to say why exactly.
>> The patch actually makes it say "matched GIT_SKIP_TESTS".
>> It looks OK on the console.
> Still just bikeshedding:
>
> That new message in patch #2 says "not in GIT_TEST_ONLY", but isn't
> "(excluded)" also applicable to that case? Is it important to be able
> to distinguish between the two "excluded" reasons?
>
> (No more bikeshedding for me.)

Makes sense.  I guess it is unlikely you would want to use both include 
and exclude filters in one run.
On the other hand it seems nice to see the reason why it was skipped.

Here are both options for comparison.  "Longer":

     $ GIT_SKIP_TESTS='t0000.[236789] t0001.??' ./t0001-init.sh
     ok 1 - plain
     ok 2 # skip plain nested in bare (matched GIT_SKIP_TESTS)
     ok 3 # skip plain through aliased command, outside any git repo 
(matched GIT_SKIP_TESTS)
     not ok 4 - plain nested through aliased command # TODO known breakage
     not ok 5 - plain nested in bare through aliased command # TODO 
known breakage
     ok 6 # skip plain with GIT_WORK_TREE (matched GIT_SKIP_TESTS)

and changed to "excluded":

     $ GIT_SKIP_TESTS='t0000.[236789] t0001.??' ./t0001-init.sh
     ok 1 - plain
     ok 2 # skip plain nested in bare (excluded)
     ok 3 # skip plain through aliased command, outside any git repo 
(excluded)
     not ok 4 - plain nested through aliased command # TODO known breakage
     not ok 5 - plain nested in bare through aliased command # TODO 
known breakage
     ok 6 # skip plain with GIT_WORK_TREE (excluded)

P.S. It seems that now the whole interface may change :)