Hi Ævar
On 06/10/2022 16:36, Ævar Arnfjörð Bjarmason wrote:
On Thu, Oct 06 2022, Phillip Wood via GitGitGadget wrote:
From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>>
test_todo() is built upon test_expect_failure() but accepts commands
starting with test_* in addition to git. As our test_* assertions use
BUG() to signal usage errors any such error will not be hidden by
test_todo().
I think they will, unless I'm missing something. E.g. try out:
It's talking about BUG() in test-lib.sh, not the C function. For example
test_path_is_file () {
test "$#" -ne 1 && BUG "1 param"
if ! test -f "$1"
then
echo "File $1 doesn't exist"
false
fi
}
So a test containing "test_todo test_path_is_file a b" should fail as
BUG calls exit rather than returning non-zero (I should probably test
that in 0000-basic.sh)
diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
index 80e76a4695e..1be895abba6 100755
--- a/t/t0210-trace2-normal.sh
+++ b/t/t0210-trace2-normal.sh
@@ -170,7 +170,7 @@ test_expect_success 'BUG messages are written to trace2' '
test_expect_success 'bug messages with BUG_if_bug() are written to trace2' '
test_when_finished "rm trace.normal actual expect" &&
- test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
+ test_todo env GIT_TRACE2="$(pwd)/trace.normal" \
test-tool trace2 008bug 2>err &&
cat >expect <<-\EOF &&
a bug message
I.e. in our tests you need to look out for exit code 99, not the usual
abort().
I have local patches to fix this, previously submitted as an RFC here:
https://lore.kernel.org/git/RFC-cover-0.3-00000000000-20220525T234908Z-avarab@xxxxxxxxx/
I just rebased that & CI is currently running, I'll see how it does:
https://github.com/avar/git/tree/avar/usage-do-not-abort-on-BUG-to-get-trace2-event-2
When I merged your patches here with that topic yours started doing the
right thing in this case, i.e. a "test_todo" that would get a BUG()"
would be reported as a failure.
+ test_true () {
+ true
+ }
+ test_expect_success "pretend we have fixed a test_todo breakage" \
+ "test_todo test_true"
"Why the indirection", until I realized that it's because you're working
around the whitelist of commands that we have, i.e. we allow 'git' and
'test-tool', but not 'grep' or whatever.
I'm of the opinion that we should just drop that limitation altogether,
which is shown to be pretty silly in this case. I.e. at some point we
listed "test_*" as "this invokes a git binary", but a lot of our test_*
commands don't, including this one.
test_expect_failure does not allow test_* are you thinking of test-tool?
So in general I think we should just allow any command in
"test_must_fail" et al, and just catch in code review if someone uses
"test_must_fail grep" as opposed to "! grep".
That is not going to scale well
But for the particular case of "test_todo" doing so seems like pointless
work, if we think we're going to miss this sort of thing in review in
general, then surely that's not a concern if we're going to very
prominently mark tests as TODO tests, given how the test of the output
shows them?
>[...]
test_might_fail () {
- test_must_fail ok=success "$@" 2>&7
+ test_must_fail_helper might_fail ok=success "$@" 2>&7
} 7>&2 2>&4
# Similar to test_must_fail and test_might_fail, but check that a
I remember finding it annoying that "test_might_fail" is misreported
from test_must_fail_acceptable as being called "test_must_fail", so this
refactoring is very welcome.
But can you please split this into its own commit? I.e. that improvement
can stand on its own, i.e. just a change that has "test_must_fail" and
"test_might_fail" reporting their correct name.
>
Then this commit can follow, and then you'll just need to add (for this part) >
test_must_fail_helper todo "$@" 2>&7
And it'll make the resulting diff much smaller & easier to follow.
Sure, I should also improve the error message in
>> + echo >&7 "test_$test_must_fail_name_: only 'git' is allowed: $*"
for test_todo.
Best Wishes
Phillip
