| Message ID | c6803df1b6afead99a0a6a383ab9aa563920f464.1655242070.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reactions to v2.37.0-rc0 test coverage report | expand |
On Tue, Jun 14, 2022 at 5:36 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > The '--verbose' option reports what is being added and removed from the > index, but has not been tested up to this point. Augment the tests in > t2107 to check the '--verbose' option in some scenarios. > > Signed-off-by: Derrick Stolee <derrickstolee@github.com> > --- > diff --git a/t/t2106-update-index-assume-unchanged.sh b/t/t2106-update-index-assume-unchanged.sh > @@ -20,7 +20,7 @@ test_expect_success 'do not switch branches with dirty file' ' > echo dirt >file && > - git update-index --assume-unchanged file && > + git update-index --verbose --assume-unchanged file && > test_must_fail git checkout - 2>err && > test_i18ngrep overwritten err > ' If this test passes with or without the addition of `--verbose`, then adding `--verbose` unnecessarily only pollutes what is (presumably) the minimum code necessary to implement what the test is checking, and may confuse future readers into thinking that something subtle is going on. > diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh > @@ -36,9 +36,14 @@ test_expect_success '--cacheinfo does not accept blob null sha1' ' > echo content >file && > git add file && > git rev-parse :file >expect && > - test_must_fail git update-index --cacheinfo 100644 $ZERO_OID file && > + test_must_fail git update-index --verbose --cacheinfo 100644 $ZERO_OID file >out && > git rev-parse :file >actual && > - test_cmp expect actual > + test_cmp expect actual && > + > + cat >expect <<-\EOF && > + add '\''file'\'' > + EOF > + test_cmp expect out > ' While I understand your desire to address a gap in the test coverage, I worry that this sort of change, which is orthogonal to the test's stated purpose, has the same downsides as mentioned above (i.e. polluting the minimum necessary code, and potentially confusing readers). Rather than piggybacking on existing tests, adding one or two new standalone tests dedicated to checking `--verbose` would be more palatable, more understandable, and be less likely to confuse future readers. The same comment applies to the remaining changes in this patch.
On 6/15/2022 7:18 PM, Eric Sunshine wrote: > On Tue, Jun 14, 2022 at 5:36 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> The '--verbose' option reports what is being added and removed from the >> index, but has not been tested up to this point. Augment the tests in >> t2107 to check the '--verbose' option in some scenarios. >> >> Signed-off-by: Derrick Stolee <derrickstolee@github.com> >> --- >> diff --git a/t/t2106-update-index-assume-unchanged.sh b/t/t2106-update-index-assume-unchanged.sh >> @@ -20,7 +20,7 @@ test_expect_success 'do not switch branches with dirty file' ' >> echo dirt >file && >> - git update-index --assume-unchanged file && >> + git update-index --verbose --assume-unchanged file && >> test_must_fail git checkout - 2>err && >> test_i18ngrep overwritten err >> ' > > If this test passes with or without the addition of `--verbose`, then > adding `--verbose` unnecessarily only pollutes what is (presumably) > the minimum code necessary to implement what the test is checking, and > may confuse future readers into thinking that something subtle is > going on. Thanks for pointing this out. I shouldn't have left this change in. >> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh >> @@ -36,9 +36,14 @@ test_expect_success '--cacheinfo does not accept blob null sha1' ' >> echo content >file && >> git add file && >> git rev-parse :file >expect && >> - test_must_fail git update-index --cacheinfo 100644 $ZERO_OID file && >> + test_must_fail git update-index --verbose --cacheinfo 100644 $ZERO_OID file >out && >> git rev-parse :file >actual && >> - test_cmp expect actual >> + test_cmp expect actual && >> + >> + cat >expect <<-\EOF && >> + add '\''file'\'' >> + EOF >> + test_cmp expect out >> ' > > While I understand your desire to address a gap in the test coverage, > I worry that this sort of change, which is orthogonal to the test's > stated purpose, has the same downsides as mentioned above (i.e. > polluting the minimum necessary code, and potentially confusing > readers). Rather than piggybacking on existing tests, adding one or > two new standalone tests dedicated to checking `--verbose` would be > more palatable, more understandable, and be less likely to confuse > future readers. The same comment applies to the remaining changes in > this patch. I understand that the test wants to test specific behavior, and that behavior is focused on certain inputs to 'git update-index', but I also think that the --verbose option presents _additional information_ about what is expected from these behaviors. It doesn't change the already-tested behavior, only enhances it. If I separate things out and only had test for --verbose, I would need to replicate many of these behaviors just for that option, which would be wasteful. In this particular case, I'm demonstrating that the --verbose mode still reports the file as added (because of the earlier 'git add file') even though the command as a whole failed due to an invalid OID. Thanks, -Stolee
diff --git a/t/t2106-update-index-assume-unchanged.sh b/t/t2106-update-index-assume-unchanged.sh index d943ddf47e0..ad2692a2979 100755 --- a/t/t2106-update-index-assume-unchanged.sh +++ b/t/t2106-update-index-assume-unchanged.sh @@ -20,7 +20,7 @@ test_expect_success 'do not switch branches with dirty file' ' git reset --hard && git checkout other && echo dirt >file && - git update-index --assume-unchanged file && + git update-index --verbose --assume-unchanged file && test_must_fail git checkout - 2>err && test_i18ngrep overwritten err ' diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh index a30b7ca6bc9..07e6de84e6d 100755 --- a/t/t2107-update-index-basic.sh +++ b/t/t2107-update-index-basic.sh @@ -36,9 +36,14 @@ test_expect_success '--cacheinfo does not accept blob null sha1' ' echo content >file && git add file && git rev-parse :file >expect && - test_must_fail git update-index --cacheinfo 100644 $ZERO_OID file && + test_must_fail git update-index --verbose --cacheinfo 100644 $ZERO_OID file >out && git rev-parse :file >actual && - test_cmp expect actual + test_cmp expect actual && + + cat >expect <<-\EOF && + add '\''file'\'' + EOF + test_cmp expect out ' test_expect_success '--cacheinfo does not accept gitlink null sha1' ' @@ -59,9 +64,14 @@ test_expect_success '--cacheinfo mode,sha1,path (new syntax)' ' git rev-parse :file >actual && test_cmp expect actual && - git update-index --add --cacheinfo "100644,$(cat expect),elif" && + git update-index --add --verbose --cacheinfo "100644,$(cat expect),elif" >out && git rev-parse :elif >actual && - test_cmp expect actual + test_cmp expect actual && + + cat >expect <<-\EOF && + add '\''elif'\'' + EOF + test_cmp expect out ' test_expect_success '.lock files cleaned up' ' @@ -74,7 +84,8 @@ test_expect_success '.lock files cleaned up' ' git config core.worktree ../../worktree && # --refresh triggers late setup_work_tree, # active_cache_changed is zero, rollback_lock_file fails - git update-index --refresh && + git update-index --refresh --verbose >out && + test_must_be_empty out && ! test -f .git/index.lock ) ' @@ -83,7 +94,15 @@ test_expect_success '--chmod=+x and chmod=-x in the same argument list' ' >A && >B && git add A B && - git update-index --chmod=+x A --chmod=-x B && + git update-index --verbose --chmod=+x A --chmod=-x B >out && + cat >expect <<-\EOF && + add '\''A'\'' + chmod +x '\''A'\'' + add '\''B'\'' + chmod -x '\''B'\'' + EOF + test_cmp expect out && + cat >expect <<-EOF && 100755 $EMPTY_BLOB 0 A 100644 $EMPTY_BLOB 0 B