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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxx/
2. https://lore.kernel.org/git/cover-0.7-00000000000-20220318T002951Z-avarab@xxxxxxxxx/
3. https://lore.kernel.org/git/patch-5.7-553670da8a9-20220318T002951Z-avarab@xxxxxxxxx/
