| Message ID | 20190126204951.42455-1-koraktor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] Add tests for describe with --work-tree | expand |
On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt <koraktor@gmail.com> wrote: > > The dirty ones are already passing, but just because describe is comparing > with the wrong working tree. > > Signed-off-by: Sebastian Staudt <koraktor@gmail.com> > --- > t/t6120-describe.sh | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > index d639d94696..9a6bd1541f 100755 > --- a/t/t6120-describe.sh > +++ b/t/t6120-describe.sh > @@ -28,6 +28,24 @@ check_describe () { > ' > } > > +check_describe_worktree () { > + cd "$TEST_DIRECTORY" Strange alignment. We normally do it in a subshell... > + expect="$1" > + shift > + R=$(git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe "$@" 2>err.actual) These commands should be executed inside test_expect_success, not outside. And you need to chain commands with && to make sure if something breaks, then the whole test will fai. If it's too ugly to generate test_expect_success with a shell function, then just write a shell function that "describe" and compare (i.e. the test body). Then you can write something like this later test_expect_sucesss 'describe with --worktree foo' ' check_describe_worktree foo ' and check_describe_worktree can now do ( cd "$TEST_DIRECTORY" && .... ) > + S=$? > + cat err.actual >&3 > + test_expect_success "describe with --work-tree $*" ' > + test $S = 0 && > + case "$R" in > + $expect) echo happy ;; > + *) echo "Oops - $R is not $expect"; > + false ;; > + esac > + ' > + cd "$TRASH_DIRECTORY" > +} > + > test_expect_success setup ' > > test_tick && > @@ -145,14 +163,20 @@ check_describe A-* HEAD > > check_describe "A-*[0-9a-f]" --dirty > > +check_describe_worktree "A-*[0-9a-f]" --dirty > + > test_expect_success 'set-up dirty work tree' ' > echo >>file > ' > > check_describe "A-*[0-9a-f]-dirty" --dirty > > +check_describe_worktree "A-*[0-9a-f]-dirty" --dirty > + > check_describe "A-*[0-9a-f].mod" --dirty=.mod > > +check_describe_worktree "A-*[0-9a-f].mod" --dirty=.mod > + > test_expect_success 'describe --dirty HEAD' ' > test_must_fail git describe --dirty HEAD > ' > -- > 2.20.1 >
Am So., 27. Jan. 2019 um 01:07 Uhr schrieb Duy Nguyen <pclouds@gmail.com>: > > On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt <koraktor@gmail.com> wrote: > > > > The dirty ones are already passing, but just because describe is comparing > > with the wrong working tree. > > > > Signed-off-by: Sebastian Staudt <koraktor@gmail.com> > > --- > > t/t6120-describe.sh | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > > index d639d94696..9a6bd1541f 100755 > > --- a/t/t6120-describe.sh > > +++ b/t/t6120-describe.sh > > @@ -28,6 +28,24 @@ check_describe () { > > ' > > } > > > > +check_describe_worktree () { > > + cd "$TEST_DIRECTORY" > > Strange alignment. We normally do it in a subshell... Sure, will fix this. > > > + expect="$1" > > + shift > > + R=$(git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe "$@" 2>err.actual) > > These commands should be executed inside test_expect_success, not > outside. And you need to chain commands with && to make sure if > something breaks, then the whole test will fai. > > If it's too ugly to generate test_expect_success with a shell > function, then just write a shell function that "describe" and compare > (i.e. the test body). Then you can write something like this later > > test_expect_sucesss 'describe with --worktree foo' ' > check_describe_worktree foo > ' > > and check_describe_worktree can now do > > ( cd "$TEST_DIRECTORY" && .... ) > > My function is a modified version of check_describe(). Which does the same thing. I‘m not really experienced in Shell programming, so I didn‘t see a cleaner way. But having the cd commands in the && chain looks broken as it would break the following tests when one test fails and the code was executed in the wrong directory afterwards. > > > + S=$? > > + cat err.actual >&3 > > + test_expect_success "describe with --work-tree $*" ' > > + test $S = 0 && > > + case "$R" in > > + $expect) echo happy ;; > > + *) echo "Oops - $R is not $expect"; > > + false ;; > > + esac > > + ' > > + cd "$TRASH_DIRECTORY" > > +} > > + > > test_expect_success setup ' > > > > test_tick && > > @@ -145,14 +163,20 @@ check_describe A-* HEAD > > > > check_describe "A-*[0-9a-f]" --dirty > > > > +check_describe_worktree "A-*[0-9a-f]" --dirty > > + > > test_expect_success 'set-up dirty work tree' ' > > echo >>file > > ' > > > > check_describe "A-*[0-9a-f]-dirty" --dirty > > > > +check_describe_worktree "A-*[0-9a-f]-dirty" --dirty > > + > > check_describe "A-*[0-9a-f].mod" --dirty=.mod > > > > +check_describe_worktree "A-*[0-9a-f].mod" --dirty=.mod > > + > > test_expect_success 'describe --dirty HEAD' ' > > test_must_fail git describe --dirty HEAD > > ' > > -- > > 2.20.1 > > > > > -- > Duy
On Sun, Jan 27, 2019 at 08:13:51AM +0100, Sebastian Staudt wrote: > Am So., 27. Jan. 2019 um 01:07 Uhr schrieb Duy Nguyen <pclouds@gmail.com>: > > > > On Sun, Jan 27, 2019 at 3:51 AM Sebastian Staudt <koraktor@gmail.com> wrote: > > > > > > The dirty ones are already passing, but just because describe is comparing > > > with the wrong working tree. > > > > > > Signed-off-by: Sebastian Staudt <koraktor@gmail.com> > > > --- > > > t/t6120-describe.sh | 24 ++++++++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > > > index d639d94696..9a6bd1541f 100755 > > > --- a/t/t6120-describe.sh > > > +++ b/t/t6120-describe.sh > > > @@ -28,6 +28,24 @@ check_describe () { > > > ' > > > } > > > > > > +check_describe_worktree () { > > > + cd "$TEST_DIRECTORY" > > > > Strange alignment. We normally do it in a subshell... > > Sure, will fix this. > > > > > > + expect="$1" > > > + shift > > > + R=$(git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe "$@" 2>err.actual) > > > > These commands should be executed inside test_expect_success, not > > outside. And you need to chain commands with && to make sure if > > something breaks, then the whole test will fai. > > > > If it's too ugly to generate test_expect_success with a shell > > function, then just write a shell function that "describe" and compare > > (i.e. the test body). Then you can write something like this later > > > > test_expect_sucesss 'describe with --worktree foo' ' > > check_describe_worktree foo > > ' > > > > and check_describe_worktree can now do > > > > ( cd "$TEST_DIRECTORY" && .... ) > > > > > > My function is a modified version of check_describe(). Whoa. That function is 12 years old! I think our style has evolved a bit since then. > Which does the same thing. I‘m not really experienced in Shell > programming, so I didn‘t see a cleaner way. > > But having the cd commands in the && chain looks broken as it would > break the following tests when one test fails and the code was executed > in the wrong directory afterwards. I mean chaining within a test. This is to make sure any failure triggers the test failure (as it should, if some command is expected to fail, we have other ways to catch it). I would start with something simple, not using shell function at all. Something like this as an example (I added run_describe() because that "git" command becomes too long). Have a look at the "do's and don'ts" in t/README too. -- 8< -- diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index d639d94696..646bedf4e9 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -28,6 +28,10 @@ check_describe () { ' } +run_describe() { + git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe "$@" +} + test_expect_success setup ' test_tick && @@ -145,6 +149,14 @@ check_describe A-* HEAD check_describe "A-*[0-9a-f]" --dirty +test_expect_success 'describe with --work-tree --dirty' ' + ( + cd "$TEST_DIRECTORY" && + run_describe --dirty 2>err.actual >actual && + grep "^A-.*[0-9a-f]$" actual + ) +' + test_expect_success 'set-up dirty work tree' ' echo >>file ' -- 8< -- BTW, careful about _success or _failure. You need to make sure bisect is not broken. If you add a test to confirm a broken case then it should be test_expect_failure (and the test suite will pass). Then when you fix it you can flip it to test_expect_success. -- Duy
Duy Nguyen <pclouds@gmail.com> writes: >> My function is a modified version of check_describe(). > > Whoa. That function is 12 years old! I think our style has evolved a > bit since then. ;-). > I mean chaining within a test. This is to make sure any failure > triggers the test failure (as it should, if some command is expected > to fail, we have other ways to catch it). > > I would start with something simple, not using shell function at > all. Something like this as an example (I added run_describe() because > that "git" command becomes too long). Have a look at the "do's and > don'ts" in t/README too. Thanks for guiding new contributors with an easy to understand example. > BTW, careful about _success or _failure. You need to make sure bisect > is not broken. If you add a test to confirm a broken case then it > should be test_expect_failure (and the test suite will pass). Then > when you fix it you can flip it to test_expect_success. And if the fix is simple enough (i.e. a good rule of thumb is if the fixes themselves without tests need to be multi-patch series, it is not simple enough), have a single patch that has both fix and test that expects success, instead of splitting them into two to make a two patch series whose first step expects a failure and whose second step fixes and flips failure to success.
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index d639d94696..9a6bd1541f 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -28,6 +28,24 @@ check_describe () { ' } +check_describe_worktree () { + cd "$TEST_DIRECTORY" + expect="$1" + shift + R=$(git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe "$@" 2>err.actual) + S=$? + cat err.actual >&3 + test_expect_success "describe with --work-tree $*" ' + test $S = 0 && + case "$R" in + $expect) echo happy ;; + *) echo "Oops - $R is not $expect"; + false ;; + esac + ' + cd "$TRASH_DIRECTORY" +} + test_expect_success setup ' test_tick && @@ -145,14 +163,20 @@ check_describe A-* HEAD check_describe "A-*[0-9a-f]" --dirty +check_describe_worktree "A-*[0-9a-f]" --dirty + test_expect_success 'set-up dirty work tree' ' echo >>file ' check_describe "A-*[0-9a-f]-dirty" --dirty +check_describe_worktree "A-*[0-9a-f]-dirty" --dirty + check_describe "A-*[0-9a-f].mod" --dirty=.mod +check_describe_worktree "A-*[0-9a-f].mod" --dirty=.mod + test_expect_success 'describe --dirty HEAD' ' test_must_fail git describe --dirty HEAD '
The dirty ones are already passing, but just because describe is comparing with the wrong working tree. Signed-off-by: Sebastian Staudt <koraktor@gmail.com> --- t/t6120-describe.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)