From: Patrick Steinhardt <ps@pks.im>
To: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Cc: git@vger.kernel.org, "SZEDER Gábor" <szeder.dev@gmail.com>,
"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH 1/5] t1300: write test expectations in the test's body
Date: Mon, 15 Sep 2025 13:41:18 +0200 [thread overview]
Message-ID: <aMf7XvXasOiUAaxo@pks.im> (raw)
In-Reply-To: <94eb9052-18e1-4565-8f33-42fdf136e2a4@app.fastmail.com>
On Thu, Sep 11, 2025 at 06:50:29PM +0200, Kristoffer Haugsbakk wrote:
> On Thu, Sep 11, 2025, at 15:24, Patrick Steinhardt wrote:
> > There are a bunch of tests in t1300 where we write the test expectation
> > handed over to `test_cmp ()` outside of the test body. This does not
> > match our modern test style, and there isn't really a reason why this
> > would need to happen outside of the test bodies.
> >
> > Convert those to instead do so as part of the test itself.
> >
> > Note that there are two exceptions that we don't convert. In both of
> > these cases the expectation is reused across multiple tests, and thus a
> > conversion where we'd move that into the first test that uses the
> > expectation would be invalid. Those are simply left as-is for now.
>
> This is just a suggestion (which everything is):
>
> Note that there are two exceptions that we leave as-is for now since
> they are reused across tests.
Yup, that reads way better.
> > index f856821839..bde9bda234 100755
> > --- a/t/t1300-config.sh
> > +++ b/t/t1300-config.sh
> > @@ -134,38 +134,39 @@ test_expect_success 'clear default config' '
> > rm -f .git/config
> > '
> >
> > -cat > expect << EOF > +test_expect_success 'initial' '
> > + cat >expect <<EOF &&
> > [section]
> > penguin = little blue
> > EOF
> > -test_expect_success 'initial' '
>
> Ok. Correct. But I see that you are also overall doing a sort of
> normalization.
>
> • Remove-tabs if it makes sense
> • No variable expansion if it makes sense
>
> And here `<<\EOF` would work too. Or is that worse style?
>
> I will keep mentioning this throughout the rest.
No, it's not. I already did it sometimes, but not consistently. I'll
mention this in the commit message and adapt as possible.
> > @@ -214,7 +213,9 @@ test_expect_success 'Prohibited LF in comment' '
> > test_must_fail git config ${mode_set} --comment="a${LF}b" section.k v
> > '
> >
> > -test_expect_success 'non-match result' 'test_cmp expect .git/config'
> > +test_expect_success 'non-match result' '
> > + test_cmp expect .git/config
> > +'
>
> Okay. You normalize the one line to
>
> test_expect_success <name> '
> <body>
> '
>
> The same kind of change done in the next patch `t1300: small
> style fixups`.
True, will move into the next patch.
Thanks for going through all of these!
Patrick
next prev parent reply other threads:[~2025-09-15 11:41 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-11 13:24 [PATCH 0/5] builtin/config: bug fixes for "get" subcommand with "--type=color" Patrick Steinhardt
2025-09-11 13:24 ` [PATCH 1/5] t1300: write test expectations in the test's body Patrick Steinhardt
2025-09-11 16:50 ` Kristoffer Haugsbakk
2025-09-15 11:41 ` Patrick Steinhardt [this message]
2025-09-11 13:24 ` [PATCH 2/5] t1300: small style fixups Patrick Steinhardt
2025-09-11 13:24 ` [PATCH 3/5] builtin/config: do not die in `get_color()` Patrick Steinhardt
2025-09-11 13:24 ` [PATCH 4/5] builtin/config: special-case retrieving colors without a key Patrick Steinhardt
2025-09-11 16:48 ` Kristoffer Haugsbakk
2025-09-15 11:41 ` Patrick Steinhardt
2025-09-11 13:24 ` [PATCH 5/5] builtin/config: do not spawn pager when printing color codes Patrick Steinhardt
2025-09-11 16:49 ` Kristoffer Haugsbakk
2025-09-15 12:52 ` [PATCH v2 0/5] builtin/config: bug fixes for "get" subcommand with "--type=color" Patrick Steinhardt
2025-09-15 12:52 ` [PATCH v2 1/5] t1300: write test expectations in the test's body Patrick Steinhardt
2025-09-15 12:52 ` [PATCH v2 2/5] t1300: small style fixups Patrick Steinhardt
2025-09-15 12:52 ` [PATCH v2 3/5] builtin/config: do not die in `get_color()` Patrick Steinhardt
2025-09-15 12:52 ` [PATCH v2 4/5] builtin/config: special-case retrieving colors without a key Patrick Steinhardt
2025-09-15 17:19 ` Junio C Hamano
2025-09-15 12:52 ` [PATCH v2 5/5] builtin/config: do not spawn pager when printing color codes Patrick Steinhardt
2025-09-15 17:28 ` Junio C Hamano
2025-09-16 6:56 ` Kristoffer Haugsbakk
2025-09-18 6:03 ` Patrick Steinhardt
2025-09-18 6:14 ` [PATCH v3 0/5] builtin/config: bug fixes for "get" subcommand with "--type=color" Patrick Steinhardt
2025-09-18 6:14 ` [PATCH v3 1/5] t1300: write test expectations in the test's body Patrick Steinhardt
2025-09-18 6:14 ` [PATCH v3 2/5] t1300: small style fixups Patrick Steinhardt
2025-09-18 6:14 ` [PATCH v3 3/5] builtin/config: do not die in `get_color()` Patrick Steinhardt
2025-09-18 6:14 ` [PATCH v3 4/5] builtin/config: special-case retrieving colors without a key Patrick Steinhardt
2025-09-18 6:49 ` Junio C Hamano
2025-09-22 13:04 ` Patrick Steinhardt
2025-09-22 16:29 ` Junio C Hamano
2025-09-18 6:14 ` [PATCH v3 5/5] builtin/config: do not spawn pager when printing color codes Patrick Steinhardt
2025-09-22 13:06 ` [PATCH v4 0/5] builtin/config: bug fixes for "get" subcommand with "--type=color" Patrick Steinhardt
2025-09-22 13:06 ` [PATCH v4 1/5] t1300: write test expectations in the test's body Patrick Steinhardt
2025-09-22 13:06 ` [PATCH v4 2/5] t1300: small style fixups Patrick Steinhardt
2025-09-22 13:06 ` [PATCH v4 3/5] builtin/config: do not die in `get_color()` Patrick Steinhardt
2025-09-22 13:06 ` [PATCH v4 4/5] builtin/config: special-case retrieving colors without a key Patrick Steinhardt
2025-09-22 13:06 ` [PATCH v4 5/5] builtin/config: do not spawn pager when printing color codes Patrick Steinhardt
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aMf7XvXasOiUAaxo@pks.im \
--to=ps@pks.im \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=kristofferhaugsbakk@fastmail.com \
--cc=szeder.dev@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.
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 NNTP newsgroup(s).