Re: [PATCH 5/5] test-lib: add support for GIT_TODO_TESTS

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Tue, Nov 27, 2018 at 11:54:45PM +0100, Ævar Arnfjörð Bjarmason wrote:
> As noted in the updated t/README documentation being added here
> setting this new GIT_TODO_TESTS variable is usually better than
> GIT_SKIP_TESTS.

I don't see why this is "usually better".  I get how it can help your
particular use-case described below, but that doesn't mean that it's
"usually better".

> My use-case for this is to get feedback from the CI infrastructure[1]
> about which tests are passing due to fixes that have trickled into
> git.git.
> 
> With the GIT_SKIP_TESTS variable this use-case is painful, you need to
> do an occasional manual run without GIT_SKIP_TESTS set. It's much
> better to use GIT_TODO_TESTS and get a report of passing TODO tests
> from prove(1) at the bottom of the test output. Once those passing
> TODO tests have trickled down to 'master' the relevant glob (set for
> all of master/next/pu) can be removed.

Neither from the commit message nor from the documentation is it clear
to me what the result of 'make test' will be when a test listed in
GIT_TODO_TESTS fails.

> As seen in the "GIT_TODO_TESTS mixed failure" test the lack of
> interaction with existing tests marked as TODO by the test suite
> itself is intentional. There's no need to print out something saying
> they matched GIT_TODO_TESTS if they're already TODO tests.
> 
> 1. https://public-inbox.org/git/875zwm15k2.fsf@evledraar.gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  ci/lib-travisci.sh |  2 +-
>  t/README           | 18 +++++++++--
>  t/t0000-basic.sh   | 81 +++++++++++++++++++++++++++++++++++++++-------
>  t/test-lib.sh      | 31 +++++++++++++++---
>  4 files changed, 112 insertions(+), 20 deletions(-)
> 
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index 69dff4d1ec..ad8290bfdb 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -121,7 +121,7 @@ osx-clang|osx-gcc)
>  	# t9810 occasionally fails on Travis CI OS X
>  	# t9816 occasionally fails with "TAP out of sequence errors" on
>  	# Travis CI OS X
> -	export GIT_SKIP_TESTS="t9810 t9816"
> +	export GIT_TODO_TESTS="t9810 t9816"

This change is not mentioned in the commit message.

As noted in the hunk's context, these test scripts are not skipped
because they don't work on OSX at all, but because they are flaky.
Consequently, reporting them as "maybe should be un-TODO'd" when they
happen to succeed is pointless and will just lead to confusion, so
this seems to be a case when GIT_TODO_TESTS is actually worse than
GIT_SKIP_TESTS.

If a failing test in GIT_TODO_TESTS makes the whole 'make test' fail,
then this should be most definitely left as GIT_SKIP_TESTS.

>  	;;
>  GIT_TEST_GETTEXT_POISON)
>  	export GIT_TEST_GETTEXT_POISON=YesPlease
> diff --git a/t/README b/t/README
> index c03b268813..922b4fb3bf 100644
> --- a/t/README
> +++ b/t/README
> @@ -207,8 +207,19 @@ ideally be reported as bugs and fixed, or guarded by a prerequisite
>  (see "Using test prerequisites" below). But until then they can be
>  skipped.
>  
> -To skip tests, set the GIT_SKIP_TESTS variable. Individual tests can
> -be skipped:
> +To skip tests, set either the GIT_SKIP_TESTS or GIT_TODO_TESTS
> +variables. The difference is that with SKIP the tests won't be run at
> +all, whereas they will be run with TODO, but in success or failure
> +annotated as a TODO test.

This is confusing.  "To skip" a test means that the test is not run at
all.  Now, if GIT_TODO_TESTS were to run the listed tests, then it
definitely won't skip them, so this sentence contradicts the previous
one.

What does "annotated as a TODO test" mean?  Something similar to how
'test_expect_failure' works?

> +It's usually preferrable to use TODO, since the test suite will report
> +those tests that unexpectedly succeed, which may indicate that
> +whatever bug broke the test in the past has been fixed, and the test
> +should be un-TODO'd. There's no such feedback loop with
> +GIT_SKIP_TESTS.
> +
> +The GIT_SKIP_TESTS and GIT_TODO_TESTS take the same values. Individual
> +tests can be skipped:
>  
>      $ GIT_SKIP_TESTS=t9200.8 sh ./t9200-git-cvsexport-commit.sh
>  
> @@ -223,7 +234,8 @@ patterns that tells which tests to skip, and either can match the
>  
>  For an individual test suite --run could be used to specify that
>  only some tests should be run or that some tests should be
> -excluded from a run.
> +excluded from a run. The --run option is a shorthand for setting
> +a GIT_SKIP_TESTS pattern.
>  
>  The argument for --run is a list of individual test numbers or
>  ranges with an optional negation prefix that define what tests in