Re: [PATCH v2 1/3] Add tests for describe with --work-tree
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Jan 28, 2019, 10:06 UTC
- Message-ID
- <20190128100659.GA6257@ash>
- In-Reply-To
- <CA+xP2SbLwmeWpqmjCiqojra5Mwrbok7sjvUsvCsaAo6XsWBbtA@mail.gmail.com>
On Sun, Jan 27, 2019 at 08:13:51AM +0100, Sebastian Staudt wrote:
Show 51 quoted lines
> 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.
Show 6 quoted lines
> 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