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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 6, 2022, 16:42 UTC
Message-ID: <c3f4a79c-2dc6-fbf4-fc61-591ebf417682@dunelm.org.uk>
In-Reply-To: <221006.864jwhrldr.gmgdl@evledraar.gmail.com>

On 06/10/2022 16:56, Ævar Arnfjörð Bjarmason wrote:

Show 25 quoted lines

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

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

Best Wishes

Phillip

Show 31 quoted lines

> Which is why I ended up doing my verison of this sort of thing as [3],
> i.e. you'd need to assert what specific exit code you expected, or
> equivalent.
> 
> But at this point in the series:
> 	
> 	diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> 	index fa7831c0674..086eaf91351 100755
> 	--- a/t/t3600-rm.sh
> 	+++ b/t/t3600-rm.sh
> 	@@ -801,7 +801,7 @@ test_expect_success SYMLINKS 'rm across a symlinked leading path (w/ index)' '
> 	 	test_todo test_must_fail git rm d/f &&
> 	 	test_todo git rev-parse --verify :d/f &&
> 	 	test -h d &&
> 	-	test_todo test_path_is_file e/f
> 	+	test_todo test_path_is_file blah
> 	 '
> 	
> 	 test_expect_success 'setup for testing rm messages' '
> 
> So, for our own test_path_* helpers we're not going to care at all, and
> any failure will do (including a missing file), but we will care for
> grep?
> 
> I'm obviously more on the "let's care" side, I just find it odd that
> you've picked this halfway point here, but not for other things you're
> wrapping.
> 
> 1. https://lore.kernel.org/git/221006.868rltrltu.gmgdl@evledraar.gmail.com/
> 2. https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@gmail.com/
> 3. https://lore.kernel.org/git/patch-5.7-553670da8a9-20220318T002951Z-avarab@gmail.com/