| Message ID | 20220222114313.14921-2-shivam828787@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | microproject: avoid using pipes in test | expand |
On 2/22/2022 6:43 AM, Shubham Mishra wrote: > pipes doesn't care about error codes and ignore them thus we should not use them in tests. > As an easy alternative, I am using a tmp file to write from git command so we can test the exit code. Please be careful about the length of your lines in your commit message. vim should auto-wrap as you write. There are also some grammar issues, so here is an update to what you wrote: Pipes ignore error codes and thus we should not use them in tests. As an alternative, use a 'tmp' file to write the Git output so we can test the exit code. (My wrapping is probably too short here.) > Signed-off-by: Shubham Mishra <shivam828787@gmail.com> > --- > t/t0001-init.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/t/t0001-init.sh b/t/t0001-init.sh > index 3235ab4d53..9a8f209648 100755 > --- a/t/t0001-init.sh > +++ b/t/t0001-init.sh > @@ -489,11 +489,11 @@ test_expect_success 're-init from a linked worktree' ' > git worktree add ../linked-worktree && > mv .git/info/exclude expected-exclude && > cp .git/config expected-config && > - find .git/worktrees -print | sort >expected && > + find .git/worktrees -print >tmp && sort tmp >expected && Split each part of the &&-chain across lines, like this: + find .git/worktrees -print >tmp && + sort tmp >expected && > git -C ../linked-worktree init && > test_cmp expected-exclude .git/info/exclude && > test_cmp expected-config .git/config && > - find .git/worktrees -print | sort >actual && > + find .git/worktrees -print >tmp && sort tmp >actual && Here, too. I could make the same comments on patch 2, but I think you have enough info to go on. Thanks, -Stolee
> > pipes doesn't care about error codes and ignore them thus we should not use them in tests. > > As an easy alternative, I am using a tmp file to write from git command so we can test the exit code. > > Please be careful about the length of your lines in your > commit message. vim should auto-wrap as you write. Oops, I mostly use Nano, I think it's time to use vim. I will take care of it. > There are also some grammar issues, so here is an update > to what you wrote: > > Pipes ignore error codes and thus we should not use them > in tests. As an alternative, use a 'tmp' file to write the > Git output so we can test the exit code. Thanks for pointing this out. I will update it. I am a non-native English speaker, and looking to improve my command in language with time :) > Split each part of the &&-chain across lines, like this: > > + find .git/worktrees -print >tmp && > + sort tmp >expected && > > > git -C ../linked-worktree init && > > test_cmp expected-exclude .git/info/exclude && > > test_cmp expected-config .git/config && > > - find .git/worktrees -print | sort >actual && > > + find .git/worktrees -print >tmp && sort tmp >actual && > > Here, too. > > I could make the same comments on patch 2, but I think you > have enough info to go on. Sure, I get it! I will consider all these suggestions in v2. Thanks, Shubham
On Tue, Feb 22, 2022 at 3:08 PM Shubham Mishra <shivam828787@gmail.com> wrote: > > pipes doesn't care about error codes and ignore them thus we should not use them in tests. Only the exit code of the command before the pipe is ignored. Also it's ok to use pipes if the command before the pipe is not `git`. We trust regular commands to just work and we test only `git`. > As an easy alternative, I am using a tmp file to write from git command so we can test the exit code. In general, when improving the code in a way that has already been used by others, it's a good idea to take a look at previous commits doing the same thing. See for example 66c0c44df6 (t0000: avoid masking git exit value through pipes, 2021-09-16). I am not saying that you should copy paste the commit message though. > Signed-off-by: Shubham Mishra <shivam828787@gmail.com> > --- > t/t0001-init.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/t/t0001-init.sh b/t/t0001-init.sh > index 3235ab4d53..9a8f209648 100755 > --- a/t/t0001-init.sh > +++ b/t/t0001-init.sh > @@ -489,11 +489,11 @@ test_expect_success 're-init from a linked worktree' ' > git worktree add ../linked-worktree && > mv .git/info/exclude expected-exclude && > cp .git/config expected-config && > - find .git/worktrees -print | sort >expected && > + find .git/worktrees -print >tmp && sort tmp >expected && Please put the `find` and `sort` commands on 2 different lines when they are separated with &&. > git -C ../linked-worktree init && > test_cmp expected-exclude .git/info/exclude && > test_cmp expected-config .git/config && > - find .git/worktrees -print | sort >actual && > + find .git/worktrees -print >tmp && sort tmp >actual && Idem. > test_cmp expected actual > ) > ' > -- > 2.25.1 >
Christian Couder <christian.couder@gmail.com> writes: > Only the exit code of the command before the pipe is ignored. > > Also it's ok to use pipes if the command before the pipe is not `git`. > We trust regular commands to just work and we test only `git`. Both very good points. So ... >> - find .git/worktrees -print | sort >expected && >> + find .git/worktrees -print >tmp && sort tmp >expected && > > Please put the `find` and `sort` commands on 2 different lines when > they are separated with &&. We do not want to split this pipeline into two commands.
On Wed, Feb 23, 2022 at 10:07 PM Junio C Hamano <gitster@pobox.com> wrote: > > Christian Couder <christian.couder@gmail.com> writes: > > > Only the exit code of the command before the pipe is ignored. > > > > Also it's ok to use pipes if the command before the pipe is not `git`. > > We trust regular commands to just work and we test only `git`. > > Both very good points. So ... > > >> - find .git/worktrees -print | sort >expected && > >> + find .git/worktrees -print >tmp && sort tmp >expected && > > > > Please put the `find` and `sort` commands on 2 different lines when > > they are separated with &&. > > We do not want to split this pipeline into two commands. Yeah, right.
diff --git a/t/t0001-init.sh b/t/t0001-init.sh index 3235ab4d53..9a8f209648 100755 --- a/t/t0001-init.sh +++ b/t/t0001-init.sh @@ -489,11 +489,11 @@ test_expect_success 're-init from a linked worktree' ' git worktree add ../linked-worktree && mv .git/info/exclude expected-exclude && cp .git/config expected-config && - find .git/worktrees -print | sort >expected && + find .git/worktrees -print >tmp && sort tmp >expected && git -C ../linked-worktree init && test_cmp expected-exclude .git/info/exclude && test_cmp expected-config .git/config && - find .git/worktrees -print | sort >actual && + find .git/worktrees -print >tmp && sort tmp >actual && test_cmp expected actual ) '
pipes doesn't care about error codes and ignore them thus we should not use them in tests. As an easy alternative, I am using a tmp file to write from git command so we can test the exit code. Signed-off-by: Shubham Mishra <shivam828787@gmail.com> --- t/t0001-init.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)