# [PATCH 1/2] t5318: use 'test_must_be_empty' 7 messages from 2020-06-01 to 2020-06-03. Participants: Taylor Blau, Derrick Stolee, SZEDER Gábor. Thread: https://gitlist.dev/t/53593 ## Taylor Blau, 2020-06-01 18:01 Subject: [PATCH 1/2] t5318: use 'test_must_be_empty' Message-ID: URL: https://gitlist.dev/e/fddbe588147444fe2675224c94f50e24a1d12871.1591034353.git.me%40ttaylorr.com In-Reply-To: ``` 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 --- 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 ``` ## Taylor Blau, 2020-06-01 18:01 Subject: [PATCH 2/2] t5318: test that '--stdin-commits' respects '--[no-]progress' Message-ID: URL: https://gitlist.dev/e/cc6b3749abc039f8502f490d3b7c7cad485810bc.1591034353.git.me%40ttaylorr.com In-Reply-To: ``` 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 --- 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 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 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 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 ``` ## Taylor Blau, 2020-06-01 18:01 Subject: [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Message-ID: URL: https://gitlist.dev/e/cover.1591034353.git.me%40ttaylorr.com ``` 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 ``` ## Derrick Stolee, 2020-06-01 19:35 Subject: Re: [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Message-ID: <153faa41-2801-d6bc-1f48-05a0ac08b446@gmail.com> URL: https://gitlist.dev/e/153faa41-2801-d6bc-1f48-05a0ac08b446%40gmail.com In-Reply-To: ``` 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 ``` ## Taylor Blau, 2020-06-01 19:36 Subject: Re: [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Message-ID: <20200601193648.GA32262@syl.local> URL: https://gitlist.dev/e/20200601193648.GA32262%40syl.local In-Reply-To: <153faa41-2801-d6bc-1f48-05a0ac08b446@gmail.com> ``` 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 ``` ## SZEDER Gábor, 2020-06-02 18:04 Subject: Re: [PATCH 1/2] t5318: use 'test_must_be_empty' Message-ID: <20200602180403.GA4791@szeder.dev> URL: https://gitlist.dev/e/20200602180403.GA4791%40szeder.dev In-Reply-To: ``` 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. ``` ## Taylor Blau, 2020-06-03 22:21 Subject: Re: [PATCH 1/2] t5318: use 'test_must_be_empty' Message-ID: <20200603222107.GA37471@syl.local> URL: https://gitlist.dev/e/20200603222107.GA37471%40syl.local In-Reply-To: <20200602180403.GA4791@szeder.dev> ``` 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 --- 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 ```