* [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' @ 2020-06-01 18:01 Taylor Blau 2020-06-01 18:01 ` [PATCH 1/2] t5318: use 'test_must_be_empty' Taylor Blau ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Taylor Blau @ 2020-06-01 18:01 UTC (permalink / raw) To: git; +Cc: dstolee 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] t5318: use 'test_must_be_empty' 2020-06-01 18:01 [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Taylor Blau @ 2020-06-01 18:01 ` Taylor Blau 2020-06-02 18:04 ` SZEDER Gábor 2020-06-01 18:01 ` [PATCH 2/2] t5318: test that '--stdin-commits' respects '--[no-]progress' Taylor Blau 2020-06-01 19:35 ` [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Derrick Stolee 2 siblings, 1 reply; 7+ messages in thread From: Taylor Blau @ 2020-06-01 18:01 UTC (permalink / raw) To: git; +Cc: dstolee 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] t5318: use 'test_must_be_empty' 2020-06-01 18:01 ` [PATCH 1/2] t5318: use 'test_must_be_empty' Taylor Blau @ 2020-06-02 18:04 ` SZEDER Gábor 2020-06-03 22:21 ` Taylor Blau 0 siblings, 1 reply; 7+ messages in thread From: SZEDER Gábor @ 2020-06-02 18:04 UTC (permalink / raw) To: Taylor Blau; +Cc: git, dstolee 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] t5318: use 'test_must_be_empty' 2020-06-02 18:04 ` SZEDER Gábor @ 2020-06-03 22:21 ` Taylor Blau 0 siblings, 0 replies; 7+ messages in thread From: Taylor Blau @ 2020-06-03 22:21 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Taylor Blau, git, dstolee 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] t5318: test that '--stdin-commits' respects '--[no-]progress' 2020-06-01 18:01 [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Taylor Blau 2020-06-01 18:01 ` [PATCH 1/2] t5318: use 'test_must_be_empty' Taylor Blau @ 2020-06-01 18:01 ` Taylor Blau 2020-06-01 19:35 ` [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Derrick Stolee 2 siblings, 0 replies; 7+ messages in thread From: Taylor Blau @ 2020-06-01 18:01 UTC (permalink / raw) To: git; +Cc: dstolee 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' 2020-06-01 18:01 [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Taylor Blau 2020-06-01 18:01 ` [PATCH 1/2] t5318: use 'test_must_be_empty' Taylor Blau 2020-06-01 18:01 ` [PATCH 2/2] t5318: test that '--stdin-commits' respects '--[no-]progress' Taylor Blau @ 2020-06-01 19:35 ` Derrick Stolee 2020-06-01 19:36 ` Taylor Blau 2 siblings, 1 reply; 7+ messages in thread From: Derrick Stolee @ 2020-06-01 19:35 UTC (permalink / raw) To: Taylor Blau, git; +Cc: dstolee 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' 2020-06-01 19:35 ` [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Derrick Stolee @ 2020-06-01 19:36 ` Taylor Blau 0 siblings, 0 replies; 7+ messages in thread From: Taylor Blau @ 2020-06-01 19:36 UTC (permalink / raw) To: Derrick Stolee; +Cc: Taylor Blau, git, dstolee 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-03 22:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-01 18:01 [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Taylor Blau 2020-06-01 18:01 ` [PATCH 1/2] t5318: use 'test_must_be_empty' Taylor Blau 2020-06-02 18:04 ` SZEDER Gábor 2020-06-03 22:21 ` Taylor Blau 2020-06-01 18:01 ` [PATCH 2/2] t5318: test that '--stdin-commits' respects '--[no-]progress' Taylor Blau 2020-06-01 19:35 ` [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress' Derrick Stolee 2020-06-01 19:36 ` Taylor Blau
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).