From: Phillip Wood <phillip.wood123@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages
Date: Thu, 6 Oct 2022 17:10:05 +0100 [thread overview]
Message-ID: <7306b890-641f-2c45-f610-2aa0361d6066@dunelm.org.uk> (raw)
In-Reply-To: <221006.868rltrltu.gmgdl@evledraar.gmail.com>
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@dunelm.org.uk>
>>
>> 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@gmail.com/
>
> 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
next prev parent reply other threads:[~2022-10-06 16:10 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-06 15:01 [PATCH 0/3] [RFC] tests: add test_todo() for known failures Phillip Wood via GitGitGadget
2022-10-06 15:01 ` [PATCH 1/3] [RFC] tests: add test_todo() to mark known breakages Phillip Wood via GitGitGadget
2022-10-06 15:36 ` Ævar Arnfjörð Bjarmason
2022-10-06 16:10 ` Phillip Wood [this message]
2022-10-06 20:33 ` Ævar Arnfjörð Bjarmason
2022-12-06 22:37 ` Victoria Dye
2022-12-07 12:08 ` Ævar Arnfjörð Bjarmason
2022-12-08 15:06 ` Phillip Wood
2022-12-09 1:09 ` Junio C Hamano
2022-12-09 9:04 ` Phillip Wood
2022-10-06 15:01 ` [PATCH 2/3] [RFC] test_todo: allow [!] grep as the command Phillip Wood via GitGitGadget
2022-10-06 15:56 ` Ævar Arnfjörð Bjarmason
2022-10-06 16:42 ` Phillip Wood
2022-10-06 20:26 ` Ævar Arnfjörð Bjarmason
2022-10-06 15:01 ` [PATCH 3/3] [RFC] test_todo: allow [verbose] test " Phillip Wood via GitGitGadget
2022-10-06 16:02 ` Ævar Arnfjörð Bjarmason
2022-10-06 17:05 ` [PATCH 0/3] [RFC] tests: add test_todo() for known failures Junio C Hamano
2022-10-06 19:28 ` Ævar Arnfjörð Bjarmason
2022-10-07 13:26 ` Phillip Wood
2022-10-07 17:08 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7306b890-641f-2c45-f610-2aa0361d6066@dunelm.org.uk \
--to=phillip.wood123@gmail.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).