[PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress'

[PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress'

[Taylor Blau]

Here's a short pair of patches that I wrote this morning after looking
at Stolee's most recent coverage report.

The first patch is just cleanup, and the second patch is the real
change. It would have been nice to parameterize these tests over the
arguments to 'git commit graph' (ie., have three tests for 'write',
'verify', and 'write --stdin-commits'), but '--stdin-commits' is special
since it requires input.

These patches are based off the tip of 'next', but really only need my
changes from 'tb/commit-graph-no-check-oids'.

Taylor Blau (2):
  t5318: use 'test_must_be_empty'
  t5318: test that '--stdin-commits' respects '--[no-]progress'

 t/t5318-commit-graph.sh | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

--
2.26.2.1052.gcc6b3749ab

[PATCH 1/2] t5318: use 'test_must_be_empty'

[Taylor Blau]

A handful of tests in t5318 use 'test_line_count = 0 ...' to make sure
that some command does not write any output. While correct, it is more
helpful to use 'test_must_be_empty' instead, since the latter prints the
contents of the file if it is non-empty.

Since 'test_line_count' only prints the expected and actual line count,
not the contents, using 'test_must_be_empty' may be more helpful for
debugging if there is regression in any of these tests.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5318-commit-graph.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index a79c624875..d23986f603 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -147,7 +147,7 @@ test_expect_success 'Add more commits' '
 test_expect_success 'commit-graph write progress off for redirected stderr' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph write 2>err &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '
 
 test_expect_success 'commit-graph write force progress on for stderr' '
@@ -159,13 +159,13 @@ test_expect_success 'commit-graph write force progress on for stderr' '
 test_expect_success 'commit-graph write with the --no-progress option' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph write --no-progress 2>err &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '
 
 test_expect_success 'commit-graph verify progress off for redirected stderr' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph verify 2>err &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '
 
 test_expect_success 'commit-graph verify force progress on for stderr' '
@@ -177,7 +177,7 @@ test_expect_success 'commit-graph verify force progress on for stderr' '
 test_expect_success 'commit-graph verify with the --no-progress option' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph verify --no-progress 2>err &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '
 
 # Current graph structure:
-- 
2.26.2.1052.gcc6b3749ab

Re: [PATCH 1/2] t5318: use 'test_must_be_empty'

[SZEDER Gábor]

On Mon, Jun 01, 2020 at 12:01:27PM -0600, Taylor Blau wrote:
> A handful of tests in t5318 use 'test_line_count = 0 ...' to make sure
> that some command does not write any output. While correct, it is more
> helpful to use 'test_must_be_empty' instead, since the latter prints the
> contents of the file if it is non-empty.
> 
> Since 'test_line_count' only prints the expected and actual line count,
> not the contents, using 'test_must_be_empty' may be more helpful for
> debugging if there is regression in any of these tests.

These two paragraphs essentially say the same thing, so I think only
one would be sufficient, but...  Both paragraphs are wrong, because
'test_line_count' does include the content of the file on failure:

  expecting success of 9999.1 'test': 
          cat >foo <<-EOF &&
          Add
          some
          content
          EOF
          test_line_count = 0 foo
  
  test_line_count: line count for foo != 0
  Add
  some
  content
  not ok 1 - test

Having said that, I think that the change itself is good, because
'test_must_be_empty foo' is more idiomatic.

Re: [PATCH 1/2] t5318: use 'test_must_be_empty'

[Taylor Blau]

On Tue, Jun 02, 2020 at 08:04:03PM +0200, SZEDER Gábor wrote:
> On Mon, Jun 01, 2020 at 12:01:27PM -0600, Taylor Blau wrote:
> > A handful of tests in t5318 use 'test_line_count = 0 ...' to make sure
> > that some command does not write any output. While correct, it is more
> > helpful to use 'test_must_be_empty' instead, since the latter prints the
> > contents of the file if it is non-empty.
> >
> > Since 'test_line_count' only prints the expected and actual line count,
> > not the contents, using 'test_must_be_empty' may be more helpful for
> > debugging if there is regression in any of these tests.
>
> These two paragraphs essentially say the same thing, so I think only
> one would be sufficient, but...  Both paragraphs are wrong, because
> 'test_line_count' does include the content of the file on failure:
>
>   expecting success of 9999.1 'test':
>           cat >foo <<-EOF &&
>           Add
>           some
>           content
>           EOF
>           test_line_count = 0 foo
>
>   test_line_count: line count for foo != 0
>   Add
>   some
>   content
>   not ok 1 - test
>
> Having said that, I think that the change itself is good, because
> 'test_must_be_empty foo' is more idiomatic.

Sounds good. Let's use this version of the patch instead, and otherwise
I think this should be ready to go:

--- >8 ---

Subject: [PATCH] t5318: use 'test_must_be_empty'

A handful of tests in t5318 use 'test_line_count = 0 ...' to make sure
that some command does not write any output. While correct, it is more
idiomatic to use 'test_must_be_empty' instead. Switch the former
invocations to use the latter instead.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5318-commit-graph.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index a79c624875..d23986f603 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -147,7 +147,7 @@ test_expect_success 'Add more commits' '
 test_expect_success 'commit-graph write progress off for redirected stderr' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph write 2>err &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '

 test_expect_success 'commit-graph write force progress on for stderr' '
@@ -159,13 +159,13 @@ test_expect_success 'commit-graph write force progress on for stderr' '
 test_expect_success 'commit-graph write with the --no-progress option' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph write --no-progress 2>err &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '

 test_expect_success 'commit-graph verify progress off for redirected stderr' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph verify 2>err &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '

 test_expect_success 'commit-graph verify force progress on for stderr' '
@@ -177,7 +177,7 @@ test_expect_success 'commit-graph verify force progress on for stderr' '
 test_expect_success 'commit-graph verify with the --no-progress option' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph verify --no-progress 2>err &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '

 # Current graph structure:
--
2.27.0

[PATCH 2/2] t5318: test that '--stdin-commits' respects '--[no-]progress'

[Taylor Blau]

The following lines were not covered in a recent line-coverage test
against Git:

  builtin/commit-graph.c
  5b6653e5 244) progress = start_delayed_progress(
  5b6653e5 268) stop_progress(&progress);

These statements are executed when both '--stdin-commits' and
'--progress' are passed. Introduce a trio of tests that exercise various
combinations of these options to ensure that these lines are covered.

More importantly, this is exercising a (somewhat) previously-ignored
feature of '--stdin-commits', which is that it respects '--progress'.
Prior to 5b6653e523 (builtin/commit-graph.c: dereference tags in
builtin, 2020-05-13), dereferencing input from '--stdin-commits' was
done inside of commit-graph.c.

Now that an additional progress meter may be generated from outside of
commit-graph.c, add a corresponding test to make sure that it also
respects '--[no]-progress'.

The other location that generates progress meter output (from d335ce8f24
(commit-graph.c: show progress of finding reachable commits,
2020-05-13)) is already covered by any test that passes '--reachable'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5318-commit-graph.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index d23986f603..26f332d6a3 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -162,6 +162,27 @@ test_expect_success 'commit-graph write with the --no-progress option' '
 	test_must_be_empty err
 '
 
+test_expect_success 'commit-graph write --stdin-commits progress off for redirected stderr' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git rev-parse commits/5 >in &&
+	git commit-graph write --stdin-commits <in 2>err &&
+	test_must_be_empty err
+'
+
+test_expect_success 'commit-graph write --stdin-commits force progress on for stderr' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git rev-parse commits/5 >in &&
+	GIT_PROGRESS_DELAY=0 git commit-graph write --stdin-commits --progress <in 2>err &&
+	test_i18ngrep "Collecting commits from input" err
+'
+
+test_expect_success 'commit-graph write --stdin-commits with the --no-progress option' '
+	cd "$TRASH_DIRECTORY/full" &&
+	git rev-parse commits/5 >in &&
+	git commit-graph write --stdin-commits --no-progress <in 2>err &&
+	test_must_be_empty err
+'
+
 test_expect_success 'commit-graph verify progress off for redirected stderr' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph verify 2>err &&
-- 
2.26.2.1052.gcc6b3749ab

Re: [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress'

[Derrick Stolee]

On 6/1/2020 2:01 PM, Taylor Blau wrote:
> Here's a short pair of patches that I wrote this morning after looking
> at Stolee's most recent coverage report.
> 
> The first patch is just cleanup, and the second patch is the real
> change. It would have been nice to parameterize these tests over the
> arguments to 'git commit graph' (ie., have three tests for 'write',
> 'verify', and 'write --stdin-commits'), but '--stdin-commits' is special
> since it requires input.
> 
> These patches are based off the tip of 'next', but really only need my
> changes from 'tb/commit-graph-no-check-oids'.

The first patch is an obviously good patch, and it even has a good
justification in the message.

The second is also good. The case of forcing "--progress" would be
enough for covering your new-ish progress meter. Perhaps the other
tests (or at least the one specifying "--no-progress") could be
removed, but I don't feel strongly about that.

Thanks,
-Stolee

Re: [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress'

[Taylor Blau]

On Mon, Jun 01, 2020 at 03:35:31PM -0400, Derrick Stolee wrote:
> On 6/1/2020 2:01 PM, Taylor Blau wrote:
> > Here's a short pair of patches that I wrote this morning after looking
> > at Stolee's most recent coverage report.
> >
> > The first patch is just cleanup, and the second patch is the real
> > change. It would have been nice to parameterize these tests over the
> > arguments to 'git commit graph' (ie., have three tests for 'write',
> > 'verify', and 'write --stdin-commits'), but '--stdin-commits' is special
> > since it requires input.
> >
> > These patches are based off the tip of 'next', but really only need my
> > changes from 'tb/commit-graph-no-check-oids'.
>
> The first patch is an obviously good patch, and it even has a good
> justification in the message.

Thanks.

> The second is also good. The case of forcing "--progress" would be
> enough for covering your new-ish progress meter. Perhaps the other
> tests (or at least the one specifying "--no-progress") could be
> removed, but I don't feel strongly about that.

Yeah, I don't feel strongly either. I figured that it would at least be
more consistent with the surrounding tests to have the three variants. I
guess we can see what others think, too.

> Thanks,
> -Stolee

Thanks,
Taylor