Re: [PATCH v2 00/15] t5520: various test cleanup
Sorry for the double-send. public-inbox and MARC both showed that only 3 messages got through the in the first send so I sent it again. Seems to have gotten through find the second time around. On Fri, Oct 18, 2019 at 03:04:03PM -0700, Denton Liu wrote: > Like earlier patchsets, I want to implement a feature that involves > modifications to the test suite. Since that feature will probably take a > while to polish up, however, let's clean up the test suite in a separate > patchset first so it's not blocked by the feature work. > > 1/15 is a cleanup to an unrelated test that I found while addressing > some of Eric's comments. > > 2/15 is a general improvement to test_rev_cmp() that will be used later > in the series. > > Changes since v1: > > * Incorporate Eric's feedback > > Denton Liu (15): > t7408: replace `test_must_fail test_path_is_file` > t: teach test_cmp_rev to accept ! for not-equals > t5520: improve test style > t5520: use sq for test case names > t5520: let sed open its own input > t5520: replace test -f with test-lib functions > t5520: remove spaces after redirect operator > t5520: use test_line_count where possible > t5520: replace test -{n,z} with test-lib functions > t5520: use test_cmp_rev where possible > t5520: test single-line files by git with test_cmp > t5520: don't put git in upstream of pipe > t5520: replace subshell cat comparison with test_cmp > t5520: remove redundant lines in test cases > t5520: replace `! git` with `test_must_fail git` > > t/t2400-worktree-add.sh | 4 +- > t/t3400-rebase.sh | 2 +- > t/t3421-rebase-topology-linear.sh | 6 +- > t/t3430-rebase-merges.sh| 2 +- > t/t3432-rebase-fast-forward.sh | 2 +- > t/t3501-revert-cherry-pick.sh | 2 +- > t/t3508-cherry-pick-many-commits.sh | 2 +- > t/t5520-pull.sh | 343 +--- > t/t7408-submodule-reference.sh | 2 +- > t/test-lib-functions.sh | 13 +- > 10 files changed, 227 insertions(+), 151 deletions(-) > > Range-diff against v1: > -: -- > 1: 987fee4652 t7408: replace `test_must_fail > test_path_is_file` > -: -- > 2: 417e808466 t: teach test_cmp_rev to accept ! for > not-equals > 1: 0bc54dd330 = 3: 0a56980857 t5520: improve test style > 2: a5dee82ecc = 4: dfa89ba1cb t5520: use sq for test case names > 3: 58cc2fcda3 = 5: 9fac3dff83 t5520: let sed open its own input > 4: d2946208d3 ! 6: c6ca45eb17 t5520: replace test -f with > test_path_is_file > @@ Metadata > Author: Denton Liu > > ## Commit message ## > -t5520: replace test -f with test_path_is_file > +t5520: replace test -f with test-lib functions > > Although `test -f` has the same functionality as > test_path_is_file(), in > the case where test_path_is_file() fails, we get much better > debugging > -information. Replace `test -f` with test_path_is_file so that future > -developers will have a better experience debugging these test cases. > +information. > + > +Replace `test -f` with test_path_is_file() so that future developers > +will have a better experience debugging these test cases. > + > +Also, in the case of `! test -f`, not only should that path not be a > +file, it shouldn't exist at all so replace it with > +test_path_is_missing(). > > Signed-off-by: Denton Liu > > @@ t/t5520-pull.sh: test_expect_success 'pulling into void must not > create an octop > cd cloned-octopus && > test_must_fail git pull .. master master && > -! test -f file > -+test_must_fail test_path_is_file file > ++test_path_is_missing file > ) > ' > > 5: 5b4c1dd291 = 7: 830a8212ae t5520: remove spaces after redirect operator > 6: 26fea15950 = 8: 3d982230be t5520: use test_line_count where possible > 7: 3fc0354c9c ! 9: 2bca4f046d t5520: replace test -{n,z} with test-lib > functions > @@ Metadata > ## Commit message ## > t5520: replace test -{n,z} with test-lib functions > > +When wrapping a Git command in a subshell within another command, we > +throw away the Git command's exit code. In case the Git command > fails, > +we would like to know about it rather than the failure being silent. > +Extract Git commands so that their exit codes are not lost. > + > Instead of using `test -n` or `test -z`, replace them respectively > with > -invocations of test_file_not_empty() and test_must_be_empty(). > +invocations of test_file_not_empty() and test_must_be_empty() so > that we > +get better debugging information in the case of a failure. > > Signed-off-by: Denton Liu > > 8:
[PATCH v2 00/15] t5520: various test cleanup
Like earlier patchsets, I want to implement a feature that involves modifications to the test suite. Since that feature will probably take a while to polish up, however, let's clean up the test suite in a separate patchset first so it's not blocked by the feature work. 1/15 is a cleanup to an unrelated test that I found while addressing some of Eric's comments. 2/15 is a general improvement to test_rev_cmp() that will be used later in the series. Changes since v1: * Incorporate Eric's feedback Denton Liu (15): t7408: replace `test_must_fail test_path_is_file` t: teach test_cmp_rev to accept ! for not-equals t5520: improve test style t5520: use sq for test case names t5520: let sed open its own input t5520: replace test -f with test-lib functions t5520: remove spaces after redirect operator t5520: use test_line_count where possible t5520: replace test -{n,z} with test-lib functions t5520: use test_cmp_rev where possible t5520: test single-line files by git with test_cmp t5520: don't put git in upstream of pipe t5520: replace subshell cat comparison with test_cmp t5520: remove redundant lines in test cases t5520: replace `! git` with `test_must_fail git` t/t2400-worktree-add.sh | 4 +- t/t3400-rebase.sh | 2 +- t/t3421-rebase-topology-linear.sh | 6 +- t/t3430-rebase-merges.sh| 2 +- t/t3432-rebase-fast-forward.sh | 2 +- t/t3501-revert-cherry-pick.sh | 2 +- t/t3508-cherry-pick-many-commits.sh | 2 +- t/t5520-pull.sh | 343 +--- t/t7408-submodule-reference.sh | 2 +- t/test-lib-functions.sh | 13 +- 10 files changed, 227 insertions(+), 151 deletions(-) Range-diff against v1: -: -- > 1: 987fee4652 t7408: replace `test_must_fail test_path_is_file` -: -- > 2: 417e808466 t: teach test_cmp_rev to accept ! for not-equals 1: 0bc54dd330 = 3: 0a56980857 t5520: improve test style 2: a5dee82ecc = 4: dfa89ba1cb t5520: use sq for test case names 3: 58cc2fcda3 = 5: 9fac3dff83 t5520: let sed open its own input 4: d2946208d3 ! 6: c6ca45eb17 t5520: replace test -f with test_path_is_file @@ Metadata Author: Denton Liu ## Commit message ## -t5520: replace test -f with test_path_is_file +t5520: replace test -f with test-lib functions Although `test -f` has the same functionality as test_path_is_file(), in the case where test_path_is_file() fails, we get much better debugging -information. Replace `test -f` with test_path_is_file so that future -developers will have a better experience debugging these test cases. +information. + +Replace `test -f` with test_path_is_file() so that future developers +will have a better experience debugging these test cases. + +Also, in the case of `! test -f`, not only should that path not be a +file, it shouldn't exist at all so replace it with +test_path_is_missing(). Signed-off-by: Denton Liu @@ t/t5520-pull.sh: test_expect_success 'pulling into void must not create an octop cd cloned-octopus && test_must_fail git pull .. master master && - ! test -f file -+ test_must_fail test_path_is_file file ++ test_path_is_missing file ) ' 5: 5b4c1dd291 = 7: 830a8212ae t5520: remove spaces after redirect operator 6: 26fea15950 = 8: 3d982230be t5520: use test_line_count where possible 7: 3fc0354c9c ! 9: 2bca4f046d t5520: replace test -{n,z} with test-lib functions @@ Metadata ## Commit message ## t5520: replace test -{n,z} with test-lib functions +When wrapping a Git command in a subshell within another command, we +throw away the Git command's exit code. In case the Git command fails, +we would like to know about it rather than the failure being silent. +Extract Git commands so that their exit codes are not lost. + Instead of using `test -n` or `test -z`, replace them respectively with -invocations of test_file_not_empty() and test_must_be_empty(). +invocations of test_file_not_empty() and test_must_be_empty() so that we +get better debugging information in the case of a failure. Signed-off-by: Denton Liu 8: f4eb80c9ac ! 10: 1a54db1d5c t5520: use test_cmp_rev where possible @@ Commit message s/test "$(git rev-parse.* \([^)]*\))" = "$(git rev-parse \([^)]*\))"/test_cmp_rev \1 \2/ s/test \([^ ]*\) = "$(git rev-parse.* \([^)]*\))"/test_cmp_rev \1 \2/ -s/test "$(git rev-parse.* \([^)]*\))" != "$(git rev-parse.* \([^)]*\))"/test_must_fail test_cmp_rev \1 \2/ -s/test \([^ ]*\) != "$(git rev-parse.* \([^)]*\))"/test_must_fail test_cmp_rev \1 \2