From: Phillip Wood Date: Thu, 06 Oct 2022 16:42:50 GMT Subject: Re: [PATCH 2/3] [RFC] test_todo: allow [!] grep as the command Message-ID: In-Reply-To: <221006.864jwhrldr.gmgdl@evledraar.gmail.com> On 06/10/2022 16:56, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote: > >> From: Phillip Wood >> >> 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 > 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/