Re: [PATCH 2/3] [RFC] test_todo: allow [!] grep as the command

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 6, 2022, 20:26 UTC
Message-ID: <221006.86r0zkr8w2.gmgdl@evledraar.gmail.com>
In-Reply-To: <c3f4a79c-2dc6-fbf4-fc61-591ebf417682@dunelm.org.uk>

On Thu, Oct 06 2022, Phillip Wood wrote:

Show 29 quoted lines

> On 06/10/2022 16:56, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:
>> 
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> Many failing tests use grep, this commit converts a sample to use
>>> test_todo(). As POSIX specifies that a return code greater than one
>>> indicates an error rather than a failing match we take care not the
>>> hide that.
>> Ah, so on the one hand this gives me second thoughts about my stance
>> in[1], i.e. if we just allowed any command we wouldn't be forced to add
>> these sorts of special-cases.
>> Although, we could also allow any command, and just add smartness
>> for
>> ones we know about, e.g. "grep".
>> But I do find doing this to be weirdly inconsistent, i.e. caring
>> about
>> the difference between these two:
>> 	$ grep blah README.md; echo $?
>> 	1
>> 	$ grep blah README.mdx; echo $?
>> 	grep: README.mdx: No such file or directory
>> 	2
>
> The intent was to catch bad options, not missing files (i.e. we don't
> want test_todo to hide a failure from "grep --invalid-option"). We
> could check the file exists and skip running grep if it does not
> (hopefully the test wont be grepping multiple files in a single
> command)

It returns the same exit code for missing files and bad options, so I don't think this plan will work.

I.e. I (in my initial series) wanted to have something where we declared what the behavior was right now, *and* what it should be.

But some of our tests are wishy-washing "I wish this worked", so:

	test_todo git some-new-cmd && # should write "unicorn" to a new foo.txt
	test_todo grep unicorn foo.txt

Won't do what you expect?

Show 13 quoted lines

>> Is basically why I took the approach I did in my [2], i.e. to force us
>> to positively assert *what* the bad behavior should be.
>
> That is what made the end result so hard to use though
>
> 	test_todo \
> 		--want "test_must_fail git" \
> 		--reset "git reset --hard" \
> 		--expect git \
> 		-- \
> 		rm d/f &&
>
> is not exactly readable.

Yes, indeed:) Anyway, my just-sent https://lore.kernel.org/git/221006.86v8owr986.gmgdl@evledraar.gmail.com/ goes into that.

I think a "test_todo" should either be a "strictly declare stuff", or a "YOLO this" where we just detect segfaults.

But per the above having it be some mix of the two is just confusing, i.e. to extend the example above:

	test_todo git some-new-cmd &&
	test_todo test_path_exists foo.txt &&
	test_todo grep unicorn foo.txt

Won't "work", because the "test_path_exists" isn't strict, but your "grep" is.

So I think whatever "test_todo" does it should be picking one or the other.