Re: [PATCH 3/3] [RFC] test_todo: allow [verbose] test as the command

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <stolee@gmail.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH 3/3] [RFC] test_todo: allow [verbose] test as the command
Date: Thu, 06 Oct 2022 18:02:23 +0200	[thread overview]
Message-ID: <221006.86zge9q6js.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <c0d955c2d1fa234ad2e4a8aad467b991030ccf47.1665068476.git.gitgitgadget@gmail.com>

On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> For simplicity test_todo() allows verbose to precede any valid
> command.  As POSIX specifies that a return code greater than one is an
> error rather than a failed test we take care not to hide that.
>
> I'm in two minds about this patch. Generally it is better to use one
> of our test helpers such as test_cmp() rather than calling test
> directly.  There are so few instances of test being used within
> test_expect_failure() (the conversions here are not exhaustive but
> there are not many more) that it would probably be better to convert
> the tests by using the appropriate helper rather than supporting
> calling test as the command to test_todo().

I think that we might want to salvage parts of this, but we really
shouldn't be spending review time on carrying forward a bad pattern that
hides segfaults.

I.e. whatever we do about "test_todo"'s interaction with "test" let's
first change things like...

> -test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '
> +test_expect_success CASE_INSENSITIVE_FS 'add (with different case)' '
>  	git reset --hard initial &&
>  	rm camelcase &&
>  	echo 1 >CamelCase &&
> @@ -108,7 +108,7 @@ test_expect_failure CASE_INSENSITIVE_FS 'add (with different case)' '
>  	git ls-files >tmp &&
>  	camel=$(grep -i camelcase tmp) &&
>  	test $(echo "$camel" | wc -l) = 1 &&
> -	test "z$(git cat-file blob :$camel)" = z1
> +	test_todo test "z$(git cat-file blob :$camel)" = z1

...this to e.g.:

	echo z1 >expect &&
	git cat-file blob :$camel >actual &&
	test_cmp expect actual

Or whatever, then let's see if migrating "verbose" is worthwhile, in the
post-image you end up with no real users of it, only your tests.

I've wanted to just remove it for a while, all its users seem to be
either bad uses like that, or we'd get much better bang for the buck out
of it by having a t/verbose-bin/ or whatever, which would just wrap
arbitrary commands like "grep" and the like (i.e. ones where we could
provide useful context).