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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Oct 06 2022, Phillip Wood wrote:

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

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