| Message ID | 404c9186080ecee6c1cc39a6dcd17deaaa7a620a.1537243720.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use generation numbers for --topo-order | expand |
On Mon, Sep 17, 2018 at 09:08:44PM -0700, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > The 'test_three_modes' method assumes we are using the 'test-tool > reach' command for our test. However, we may want to use the data > shape of our commit graph and the three modes (no commit-graph, > full commit-graph, partial commit-graph) for other git commands. > > Split test_three_modes to be a simple translation on a more general > run_three_modes method that executes the given command and tests > the actual output to the expected output. > > While inspecting this code, I realized that the final test for > 'commit_contains --tag' is silently dropping the '--tag' argument. > It should be quoted to include both. Nit: while quoting the function's arguments does fix the issue, it leaves the tests prone to the same issue in the future. Wouldn't it be better to use $@ inside the function to refer to all its arguments? > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > t/t6600-test-reach.sh | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh > index d139a00d1d..1b18e12a4e 100755 > --- a/t/t6600-test-reach.sh > +++ b/t/t6600-test-reach.sh > @@ -53,18 +53,22 @@ test_expect_success 'setup' ' > git config core.commitGraph true > ' > > -test_three_modes () { > +run_three_modes () { > test_when_finished rm -rf .git/objects/info/commit-graph && > - test-tool reach $1 <input >actual && > + $1 <input >actual && > test_cmp expect actual && > cp commit-graph-full .git/objects/info/commit-graph && > - test-tool reach $1 <input >actual && > + $1 <input >actual && > test_cmp expect actual && > cp commit-graph-half .git/objects/info/commit-graph && > - test-tool reach $1 <input >actual && > + $1 <input >actual && > test_cmp expect actual > } > > +test_three_modes () { > + run_three_modes "test-tool reach $1" > +} > + > test_expect_success 'ref_newer:miss' ' > cat >input <<-\EOF && > A:commit-5-7 > @@ -219,7 +223,7 @@ test_expect_success 'commit_contains:hit' ' > EOF > echo "commit_contains(_,A,X,_):1" >expect && > test_three_modes commit_contains && > - test_three_modes commit_contains --tag > + test_three_modes "commit_contains --tag" > ' > > test_expect_success 'commit_contains:miss' ' > -- > gitgitgadget >
SZEDER Gábor <szeder.dev@gmail.com> writes: >> While inspecting this code, I realized that the final test for >> 'commit_contains --tag' is silently dropping the '--tag' argument. >> It should be quoted to include both. > > Nit: while quoting the function's arguments does fix the issue, it > leaves the tests prone to the same issue in the future. Wouldn't it > be better to use $@ inside the function to refer to all its arguments? IOW, do it more like this? >> -test_three_modes () { >> +run_three_modes () { >> test_when_finished rm -rf .git/objects/info/commit-graph && >> - test-tool reach $1 <input >actual && >> + $1 <input >actual && "$@" <input >actual i.e. treat each parameter as separate things without further getting split at $IFS and ... >> +test_three_modes () { >> + run_three_modes "test-tool reach $1" run_three_modes test-tool reach "$1" ... make sure there three things are sent as separate, by quoting "$1" inside dq. I think that makes sense.
Junio C Hamano <gitster@pobox.com> writes: > SZEDER Gábor <szeder.dev@gmail.com> writes: > >>> While inspecting this code, I realized that the final test for >>> 'commit_contains --tag' is silently dropping the '--tag' argument. >>> It should be quoted to include both. >> >> Nit: while quoting the function's arguments does fix the issue, it >> leaves the tests prone to the same issue in the future. Wouldn't it >> be better to use $@ inside the function to refer to all its arguments? > > IOW, do it more like this? > >>> -test_three_modes () { >>> +run_three_modes () { >>> test_when_finished rm -rf .git/objects/info/commit-graph && >>> - test-tool reach $1 <input >actual && >>> + $1 <input >actual && > > "$@" <input >actual > > i.e. treat each parameter as separate things without further getting > split at $IFS and ... > >>> +test_three_modes () { >>> + run_three_modes "test-tool reach $1" > > run_three_modes test-tool reach "$1" > > ... make sure there three things are sent as separate, by quoting > "$1" inside dq. > > I think that makes sense. I also noticed that 2/6 made "commti_contains --tag" enclosed in dq pair for one test, but the next test after it has the identical one. Here is what I queued in the meantime. diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh index 1b18e12a4e..1377849bf8 100755 --- a/t/t6600-test-reach.sh +++ b/t/t6600-test-reach.sh @@ -55,18 +55,18 @@ test_expect_success 'setup' ' run_three_modes () { test_when_finished rm -rf .git/objects/info/commit-graph && - $1 <input >actual && + "$@" <input >actual && test_cmp expect actual && cp commit-graph-full .git/objects/info/commit-graph && - $1 <input >actual && + "$@" <input >actual && test_cmp expect actual && cp commit-graph-half .git/objects/info/commit-graph && - $1 <input >actual && + "$@" <input >actual && test_cmp expect actual } test_three_modes () { - run_three_modes "test-tool reach $1" + run_three_modes test-tool reach "$1" } test_expect_success 'ref_newer:miss' ' @@ -223,7 +223,7 @@ test_expect_success 'commit_contains:hit' ' EOF echo "commit_contains(_,A,X,_):1" >expect && test_three_modes commit_contains && - test_three_modes "commit_contains --tag" + test_three_modes commit_contains --tag ' test_expect_success 'commit_contains:miss' '
Junio C Hamano <gitster@pobox.com> writes: > I also noticed that 2/6 made "commti_contains --tag" enclosed in dq > pair for one test, but the next test after it has the identical one. > > Here is what I queued in the meantime. > ... And of course, I find out that 3/6 needs a matching update after I've almost finished day's integration cycle, and need to redo the whole thing X-<. Here is a squashable update for 3/6 to match the proposed change. -- >8 -- Subject: fixup! test-reach: add rev-list tests t/t6600-test-reach.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh index 990ab56e7a..cf9179bdb8 100755 --- a/t/t6600-test-reach.sh +++ b/t/t6600-test-reach.sh @@ -252,7 +252,7 @@ test_expect_success 'rev-list: basic topo-order' ' commit-6-2 commit-5-2 commit-4-2 commit-3-2 commit-2-2 commit-1-2 \ commit-6-1 commit-5-1 commit-4-1 commit-3-1 commit-2-1 commit-1-1 \ >expect && - run_three_modes "git rev-list --topo-order commit-6-6" + run_three_modes git rev-list --topo-order commit-6-6 ' test_expect_success 'rev-list: first-parent topo-order' ' @@ -264,7 +264,7 @@ test_expect_success 'rev-list: first-parent topo-order' ' commit-6-2 \ commit-6-1 commit-5-1 commit-4-1 commit-3-1 commit-2-1 commit-1-1 \ >expect && - run_three_modes "git rev-list --first-parent --topo-order commit-6-6" + run_three_modes git rev-list --first-parent --topo-order commit-6-6 ' test_expect_success 'rev-list: range topo-order' ' @@ -276,7 +276,7 @@ test_expect_success 'rev-list: range topo-order' ' commit-6-2 commit-5-2 commit-4-2 \ commit-6-1 commit-5-1 commit-4-1 \ >expect && - run_three_modes "git rev-list --topo-order commit-3-3..commit-6-6" + run_three_modes git rev-list --topo-order commit-3-3..commit-6-6 ' test_expect_success 'rev-list: range topo-order' ' @@ -288,7 +288,7 @@ test_expect_success 'rev-list: range topo-order' ' commit-6-2 commit-5-2 commit-4-2 \ commit-6-1 commit-5-1 commit-4-1 \ >expect && - run_three_modes "git rev-list --topo-order commit-3-8..commit-6-6" + run_three_modes git rev-list --topo-order commit-3-8..commit-6-6 ' test_expect_success 'rev-list: first-parent range topo-order' ' @@ -300,7 +300,7 @@ test_expect_success 'rev-list: first-parent range topo-order' ' commit-6-2 \ commit-6-1 commit-5-1 commit-4-1 \ >expect && - run_three_modes "git rev-list --first-parent --topo-order commit-3-8..commit-6-6" + run_three_modes git rev-list --first-parent --topo-order commit-3-8..commit-6-6 ' test_expect_success 'rev-list: ancestry-path topo-order' ' @@ -310,7 +310,7 @@ test_expect_success 'rev-list: ancestry-path topo-order' ' commit-6-4 commit-5-4 commit-4-4 commit-3-4 \ commit-6-3 commit-5-3 commit-4-3 \ >expect && - run_three_modes "git rev-list --topo-order --ancestry-path commit-3-3..commit-6-6" + run_three_modes git rev-list --topo-order --ancestry-path commit-3-3..commit-6-6 ' test_expect_success 'rev-list: symmetric difference topo-order' ' @@ -324,7 +324,7 @@ test_expect_success 'rev-list: symmetric difference topo-order' ' commit-3-8 commit-2-8 commit-1-8 \ commit-3-7 commit-2-7 commit-1-7 \ >expect && - run_three_modes "git rev-list --topo-order commit-3-8...commit-6-6" + run_three_modes git rev-list --topo-order commit-3-8...commit-6-6 ' test_done
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh index d139a00d1d..1b18e12a4e 100755 --- a/t/t6600-test-reach.sh +++ b/t/t6600-test-reach.sh @@ -53,18 +53,22 @@ test_expect_success 'setup' ' git config core.commitGraph true ' -test_three_modes () { +run_three_modes () { test_when_finished rm -rf .git/objects/info/commit-graph && - test-tool reach $1 <input >actual && + $1 <input >actual && test_cmp expect actual && cp commit-graph-full .git/objects/info/commit-graph && - test-tool reach $1 <input >actual && + $1 <input >actual && test_cmp expect actual && cp commit-graph-half .git/objects/info/commit-graph && - test-tool reach $1 <input >actual && + $1 <input >actual && test_cmp expect actual } +test_three_modes () { + run_three_modes "test-tool reach $1" +} + test_expect_success 'ref_newer:miss' ' cat >input <<-\EOF && A:commit-5-7 @@ -219,7 +223,7 @@ test_expect_success 'commit_contains:hit' ' EOF echo "commit_contains(_,A,X,_):1" >expect && test_three_modes commit_contains && - test_three_modes commit_contains --tag + test_three_modes "commit_contains --tag" ' test_expect_success 'commit_contains:miss' '