| Message ID | 20230403223338.468025-6-rybak.a.v@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: fix unused files, part 2 | expand |
Hi Andrei On Tue, 4 Apr 2023 at 00:33, Andrei Rybak <rybak.a.v@gmail.com> wrote: > > test_expect_success 'test --parseopt invalid opt-spec' ' > test_write_lines x -- "=, x" >spec && > echo "fatal: missing opt-spec before option flags" >expect && > - test_must_fail git rev-parse --parseopt -- >out <spec 2>err && > + test_must_fail git rev-parse --parseopt -- <spec 2>err && > test_cmp expect err > ' This is the one that was touched by me. At the time I just cargo-culted other tests. This looks obviously correct to me For what it's worth: Acked-by: Øystein Walle <oystwa@gmail.com> Øsse
On Tue, Apr 04 2023, Andrei Rybak wrote: > Three tests in file t1502-rev-parse-parseopt.sh use three redirections > with invocation of "git rev-parse --parseopt --". All three tests > redirect standard output to file "out" and file "spec" to standard > input. Two of the tests redirect standard output a second time to file > "actual", and the third test redirects standard error to file "err". > These tests check contents of files "actual" and "err", but don't use > the files named "out" for assertions. The two tests that redirect to > standard output twice might also be confusing to the reader. > > Don't redirect standard output of "git rev-parse" to file "out" in > t1502-rev-parse-parseopt.sh to avoid creating unnecessary files. > > Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> > --- > t/t1502-rev-parse-parseopt.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh > index de1d48f3ba..dd811b7fb4 100755 > --- a/t/t1502-rev-parse-parseopt.sh > +++ b/t/t1502-rev-parse-parseopt.sh > @@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:" > |EOF > END_EXPECT > > - test_must_fail git rev-parse --parseopt -- -h >out <spec >actual && > + test_must_fail git rev-parse --parseopt -- -h <spec >actual && > test_cmp expect actual > ' > > test_expect_success 'test --parseopt invalid opt-spec' ' > test_write_lines x -- "=, x" >spec && > echo "fatal: missing opt-spec before option flags" >expect && > - test_must_fail git rev-parse --parseopt -- >out <spec 2>err && > + test_must_fail git rev-parse --parseopt -- <spec 2>err && > test_cmp expect err > ' > > @@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l > |EOF > END_EXPECT > > - test_must_fail git rev-parse --parseopt -- -h >out <spec >actual && > + test_must_fail git rev-parse --parseopt -- -h <spec >actual && > test_cmp expect actual > ' Ditto earlier comments: When we fail, we should assert what we emitted on stdout, surely this should also be a "test_must_be_empty out". (I didn't test that, but if that fails wes hould be testing whatever it is that we emit here, surely..)
On 06/04/2023 10:47, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Apr 04 2023, Andrei Rybak wrote: > >> Three tests in file t1502-rev-parse-parseopt.sh use three redirections >> with invocation of "git rev-parse --parseopt --". All three tests >> redirect standard output to file "out" and file "spec" to standard >> input. Two of the tests redirect standard output a second time to file >> "actual", and the third test redirects standard error to file "err". >> These tests check contents of files "actual" and "err", but don't use >> the files named "out" for assertions. The two tests that redirect to >> standard output twice might also be confusing to the reader. >> >> Don't redirect standard output of "git rev-parse" to file "out" in >> t1502-rev-parse-parseopt.sh to avoid creating unnecessary files. >> >> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> >> --- >> t/t1502-rev-parse-parseopt.sh | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh >> index de1d48f3ba..dd811b7fb4 100755 >> --- a/t/t1502-rev-parse-parseopt.sh >> +++ b/t/t1502-rev-parse-parseopt.sh >> @@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:" >> |EOF >> END_EXPECT >> >> - test_must_fail git rev-parse --parseopt -- -h >out <spec >actual && >> + test_must_fail git rev-parse --parseopt -- -h <spec >actual && >> test_cmp expect actual >> ' >> >> test_expect_success 'test --parseopt invalid opt-spec' ' >> test_write_lines x -- "=, x" >spec && >> echo "fatal: missing opt-spec before option flags" >expect && >> - test_must_fail git rev-parse --parseopt -- >out <spec 2>err && >> + test_must_fail git rev-parse --parseopt -- <spec 2>err && >> test_cmp expect err >> ' >> >> @@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l >> |EOF >> END_EXPECT >> >> - test_must_fail git rev-parse --parseopt -- -h >out <spec >actual && >> + test_must_fail git rev-parse --parseopt -- -h <spec >actual && >> test_cmp expect actual >> ' > > Ditto earlier comments: When we fail, we should assert what we emitted > on stdout, surely this should also be a "test_must_be_empty out". > > (I didn't test that, but if that fails wes hould be testing whatever it > is that we emit here, surely..) This patch is not like the others. File "out" is indeed empty, because file "actual" isn't empty. Redirect to "out" is overwritten by redirect to "actual".
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index de1d48f3ba..dd811b7fb4 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -302,14 +302,14 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:" |EOF END_EXPECT - test_must_fail git rev-parse --parseopt -- -h >out <spec >actual && + test_must_fail git rev-parse --parseopt -- -h <spec >actual && test_cmp expect actual ' test_expect_success 'test --parseopt invalid opt-spec' ' test_write_lines x -- "=, x" >spec && echo "fatal: missing opt-spec before option flags" >expect && - test_must_fail git rev-parse --parseopt -- >out <spec 2>err && + test_must_fail git rev-parse --parseopt -- <spec 2>err && test_cmp expect err ' @@ -339,7 +339,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l |EOF END_EXPECT - test_must_fail git rev-parse --parseopt -- -h >out <spec >actual && + test_must_fail git rev-parse --parseopt -- -h <spec >actual && test_cmp expect actual '
Three tests in file t1502-rev-parse-parseopt.sh use three redirections with invocation of "git rev-parse --parseopt --". All three tests redirect standard output to file "out" and file "spec" to standard input. Two of the tests redirect standard output a second time to file "actual", and the third test redirects standard error to file "err". These tests check contents of files "actual" and "err", but don't use the files named "out" for assertions. The two tests that redirect to standard output twice might also be confusing to the reader. Don't redirect standard output of "git rev-parse" to file "out" in t1502-rev-parse-parseopt.sh to avoid creating unnecessary files. Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> --- t/t1502-rev-parse-parseopt.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)