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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 6, 2022, 16:02 UTC
Message-ID: <221006.86zge9q6js.gmgdl@evledraar.gmail.com>
In-Reply-To: <c0d955c2d1fa234ad2e4a8aad467b991030ccf47.1665068476.git.gitgitgadget@gmail.com>

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

Show 13 quoted lines

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

Show 11 quoted lines

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