Re: [PATCH 1/5] t1300: write test expectations in the test's body
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 15, 2025, 11:41 UTC
- Message-ID
- <aMf7XvXasOiUAaxo@pks.im>
- In-Reply-To
- <94eb9052-18e1-4565-8f33-42fdf136e2a4@app.fastmail.com>
On Thu, Sep 11, 2025 at 06:50:29PM +0200, Kristoffer Haugsbakk wrote:
Show 17 quoted lines
> 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.
Show 23 quoted lines
> > 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.
Show 17 quoted lines
> > @@ -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