| Message ID | 20230403223338.468025-4-rybak.a.v@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: fix unused files, part 2 | expand |
On Tue, Apr 04 2023, Andrei Rybak wrote: > Three tests in t1300-config.sh check that "git config --get" barfs when > syntax errors are present in the config file. The tests redirect > standard output and standard error of "git config --get" to files, > "actual" and "error" correspondingly. They assert presence of an error > message in file "error". However, these tests don't use file "actual" > for assertions. > > Don't redirect standard output of "git config --get" to file "actual" in > t1300-config.sh to avoid creating unnecessary files. > > Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> > --- > t/t1300-config.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index d566729d74..8ac4531c1b 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' ' > [section] > key garbage > EOF > - test_must_fail git config --get section.key >actual 2>error && > + test_must_fail git config --get section.key 2>error && > test_i18ngrep " line 3 " error > ' > > @@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' ' > [section > key = value > EOF > - test_must_fail git config --get section.key >actual 2>error && > + test_must_fail git config --get section.key 2>error && > test_i18ngrep " line 2 " error > ' > > @@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' ' > [section] > key = "value string > EOF > - test_must_fail git config --get section.key >actual 2>error && > + test_must_fail git config --get section.key 2>error && > test_i18ngrep " line 3 " error > ' Ditto my comment on 1/6, shouldn't we instead be doing e.g.: diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 2575279ab84..df2070c2f09 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1575,7 +1575,8 @@ test_expect_success 'barf on syntax error' ' [section] key garbage EOF - test_must_fail git config --get section.key >actual 2>error && + test_must_fail git config --get section.key >out 2>error && + test_must_be_empty out && test_i18ngrep " line 3 " error ' I.e. before this we had no coverage on the error being the only output, but seemingly by mistake. Let's just assert that, rather than dropping the redirection entirely, no?
On 06/04/2023 10:38, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Apr 04 2023, Andrei Rybak wrote: > >> Three tests in t1300-config.sh check that "git config --get" barfs when >> syntax errors are present in the config file. The tests redirect >> standard output and standard error of "git config --get" to files, >> "actual" and "error" correspondingly. They assert presence of an error >> message in file "error". However, these tests don't use file "actual" >> for assertions. >> >> Don't redirect standard output of "git config --get" to file "actual" in >> t1300-config.sh to avoid creating unnecessary files. >> >> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> >> --- >> t/t1300-config.sh | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/t/t1300-config.sh b/t/t1300-config.sh >> index d566729d74..8ac4531c1b 100755 >> --- a/t/t1300-config.sh >> +++ b/t/t1300-config.sh >> @@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' ' >> [section] >> key garbage >> EOF >> - test_must_fail git config --get section.key >actual 2>error && >> + test_must_fail git config --get section.key 2>error && >> test_i18ngrep " line 3 " error >> ' >> >> @@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' ' >> [section >> key = value >> EOF >> - test_must_fail git config --get section.key >actual 2>error && >> + test_must_fail git config --get section.key 2>error && >> test_i18ngrep " line 2 " error >> ' >> >> @@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' ' >> [section] >> key = "value string >> EOF >> - test_must_fail git config --get section.key >actual 2>error && >> + test_must_fail git config --get section.key 2>error && >> test_i18ngrep " line 3 " error >> ' > > Ditto my comment on 1/6, shouldn't we instead be doing e.g.: > > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index 2575279ab84..df2070c2f09 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -1575,7 +1575,8 @@ test_expect_success 'barf on syntax error' ' > [section] > key garbage > EOF > - test_must_fail git config --get section.key >actual 2>error && > + test_must_fail git config --get section.key >out 2>error && > + test_must_be_empty out && > test_i18ngrep " line 3 " error > ' > > I.e. before this we had no coverage on the error being the only output, > but seemingly by mistake. Let's just assert that, rather than dropping > the redirection entirely, no? Here, failing invocations of "git config" are tested, and an argument, as Junio C Hamano outlined in https://lore.kernel.org/git/xmqqsfe8s56p.fsf@gitster.g/ for output of failing "git mktree", could be applied here. Thinking about it more, such assertions enforcing empty standard output for these commands might be helpful if some tools and/or scripts rely on empty standard output instead of checking the exit code. Hyrum's Law applies here, I guess.
On 06/04/2023 23:30, Andrei Rybak wrote: > On 06/04/2023 10:38, Ævar Arnfjörð Bjarmason wrote: >> >> Ditto my comment on 1/6, shouldn't we instead be doing e.g.: >> >> diff --git a/t/t1300-config.sh b/t/t1300-config.sh >> index 2575279ab84..df2070c2f09 100755 >> --- a/t/t1300-config.sh >> +++ b/t/t1300-config.sh >> @@ -1575,7 +1575,8 @@ test_expect_success 'barf on syntax error' ' >> [section] >> key garbage >> EOF >> - test_must_fail git config --get section.key >actual 2>error && >> + test_must_fail git config --get section.key >out 2>error && >> + test_must_be_empty out && >> test_i18ngrep " line 3 " error >> ' >> >> I.e. before this we had no coverage on the error being the only output, >> but seemingly by mistake. Let's just assert that, rather than dropping >> the redirection entirely, no? > > Here, failing invocations of "git config" are tested, and an argument, > as Junio C Hamano outlined in > https://lore.kernel.org/git/xmqqsfe8s56p.fsf@gitster.g/ > for output of failing "git mktree", could be applied here. > > Thinking about it more, such assertions enforcing empty standard output for > these commands might be helpful if some tools and/or scripts rely on empty > standard output instead of checking the exit code. Hyrum's Law applies > here, > I guess. There are some tests in t/t1300-config.sh that do check that standard output or standard error is empty. And I think I stumbled some other broken tests, while checking those. Test 'git config ignores pairs without count' checks that standard error (2>error) is empty. Just below it, there seems to be a copy-paste error: there are two tests titled 'git config ignores pairs with zero count'. First one doesn't check any output, but the second checks standard output, while calling the file "error" (>error). Test 'git config ignores pairs with empty count' checks >error as well. They were all introduced in d8d77153ea (config: allow specifying config entries via envvar pairs, 2021-01-12) by Patrick Steinhardt. Patrick, what do you think?
diff --git a/t/t1300-config.sh b/t/t1300-config.sh index d566729d74..8ac4531c1b 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1575,7 +1575,7 @@ test_expect_success 'barf on syntax error' ' [section] key garbage EOF - test_must_fail git config --get section.key >actual 2>error && + test_must_fail git config --get section.key 2>error && test_i18ngrep " line 3 " error ' @@ -1585,7 +1585,7 @@ test_expect_success 'barf on incomplete section header' ' [section key = value EOF - test_must_fail git config --get section.key >actual 2>error && + test_must_fail git config --get section.key 2>error && test_i18ngrep " line 2 " error ' @@ -1595,7 +1595,7 @@ test_expect_success 'barf on incomplete string' ' [section] key = "value string EOF - test_must_fail git config --get section.key >actual 2>error && + test_must_fail git config --get section.key 2>error && test_i18ngrep " line 3 " error '
Three tests in t1300-config.sh check that "git config --get" barfs when syntax errors are present in the config file. The tests redirect standard output and standard error of "git config --get" to files, "actual" and "error" correspondingly. They assert presence of an error message in file "error". However, these tests don't use file "actual" for assertions. Don't redirect standard output of "git config --get" to file "actual" in t1300-config.sh to avoid creating unnecessary files. Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> --- t/t1300-config.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)