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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.