| Message ID | patch-v4-5.6-9596702978e-20221219T101240Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c7e03b4e39d40b431764797d3a792169cd375705 |
| Headers | show |
| Series | tests: fix ignored & hidden exit codes | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Change tests that would lose the "git" exit code via a negation > pattern to: Looking good. thanks.
Hi Ævar On 19/12/2022 10:19, Ævar Arnfjörð Bjarmason wrote: > Change tests that would lose the "git" exit code via a negation > pattern to: > > - In the case of "t0055-beyond-symlinks.sh" compare against the > expected output instead. > > We could use the same pattern as in the "t3700-add.sh" below, doing > so would have the advantage that if we added an earlier test we > wouldn't need to adjust the "expect" output. > > But as "t0055-beyond-symlinks.sh" is a small and focused test (less > than 40 lines in total) let's use "test_cmp" instead. > > - For "t3700-add.sh" use "sed -n" to print the expected "bad" part, > and use "test_must_be_empty" to assert that it's not there. If we used > "grep" we'd get a non-zero exit code. > > We could use "test_expect_code 1 grep", but this is more consistent > with existing patterns in the test suite. It seems strange to use sed here, you could just keep using grep and check the output is empty if you don't want to use test_expect_code. There is also no need to redirect the input of the sed commands. Best Wishes Phillip > We can also remove a repeated invocation of "git ls-files" for the > last test that's being modified in that file, and search the > existing "files" output instead. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/t0055-beyond-symlinks.sh | 14 ++++++++++++-- > t/t3700-add.sh | 18 +++++++++++++----- > 2 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/t/t0055-beyond-symlinks.sh b/t/t0055-beyond-symlinks.sh > index 6bada370225..c3eb1158ef9 100755 > --- a/t/t0055-beyond-symlinks.sh > +++ b/t/t0055-beyond-symlinks.sh > @@ -15,12 +15,22 @@ test_expect_success SYMLINKS setup ' > > test_expect_success SYMLINKS 'update-index --add beyond symlinks' ' > test_must_fail git update-index --add c/d && > - ! ( git ls-files | grep c/d ) > + cat >expect <<-\EOF && > + a > + b/d > + EOF > + git ls-files >actual && > + test_cmp expect actual > ' > > test_expect_success SYMLINKS 'add beyond symlinks' ' > test_must_fail git add c/d && > - ! ( git ls-files | grep c/d ) > + cat >expect <<-\EOF && > + a > + b/d > + EOF > + git ls-files >actual && > + test_cmp expect actual > ' > > test_done > diff --git a/t/t3700-add.sh b/t/t3700-add.sh > index 51afbd7b24a..82dd768944f 100755 > --- a/t/t3700-add.sh > +++ b/t/t3700-add.sh > @@ -106,24 +106,32 @@ test_expect_success '.gitignore test setup' ' > > test_expect_success '.gitignore is honored' ' > git add . && > - ! (git ls-files | grep "\\.ig") > + git ls-files >files && > + sed -n "/\\.ig/p" <files >actual && > + test_must_be_empty actual > ' > > test_expect_success 'error out when attempting to add ignored ones without -f' ' > test_must_fail git add a.?? && > - ! (git ls-files | grep "\\.ig") > + git ls-files >files && > + sed -n "/\\.ig/p" <files >actual && > + test_must_be_empty actual > ' > > test_expect_success 'error out when attempting to add ignored ones without -f' ' > test_must_fail git add d.?? && > - ! (git ls-files | grep "\\.ig") > + git ls-files >files && > + sed -n "/\\.ig/p" <files >actual && > + test_must_be_empty actual > ' > > test_expect_success 'error out when attempting to add ignored ones but add others' ' > touch a.if && > test_must_fail git add a.?? && > - ! (git ls-files | grep "\\.ig") && > - (git ls-files | grep a.if) > + git ls-files >files && > + sed -n "/\\.ig/p" <files >actual && > + test_must_be_empty actual && > + grep a.if files > ' > > test_expect_success 'add ignored ones with -f' '
On 27/12/2022 16:44, Phillip Wood wrote: > On 19/12/2022 10:19, Ævar Arnfjörð Bjarmason wrote: >> - For "t3700-add.sh" use "sed -n" to print the expected "bad" part, >> and use "test_must_be_empty" to assert that it's not there. If we used >> "grep" we'd get a non-zero exit code. >> >> We could use "test_expect_code 1 grep", but this is more consistent >> with existing patterns in the test suite. > > It seems strange to use sed here, you could just keep using grep and > check the output is empty if you don't want to use test_expect_code. Sorry ignore that, using 'sed -n' means we don't have to worry about the exit code. > There is also no need to redirect the input of the sed commands. > > Best Wishes > > Phillip > >> We can also remove a repeated invocation of "git ls-files" for the >> last test that's being modified in that file, and search the >> existing "files" output instead. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> t/t0055-beyond-symlinks.sh | 14 ++++++++++++-- >> t/t3700-add.sh | 18 +++++++++++++----- >> 2 files changed, 25 insertions(+), 7 deletions(-) >> >> diff --git a/t/t0055-beyond-symlinks.sh b/t/t0055-beyond-symlinks.sh >> index 6bada370225..c3eb1158ef9 100755 >> --- a/t/t0055-beyond-symlinks.sh >> +++ b/t/t0055-beyond-symlinks.sh >> @@ -15,12 +15,22 @@ test_expect_success SYMLINKS setup ' >> test_expect_success SYMLINKS 'update-index --add beyond symlinks' ' >> test_must_fail git update-index --add c/d && >> - ! ( git ls-files | grep c/d ) >> + cat >expect <<-\EOF && >> + a >> + b/d >> + EOF >> + git ls-files >actual && >> + test_cmp expect actual >> ' >> test_expect_success SYMLINKS 'add beyond symlinks' ' >> test_must_fail git add c/d && >> - ! ( git ls-files | grep c/d ) >> + cat >expect <<-\EOF && >> + a >> + b/d >> + EOF >> + git ls-files >actual && >> + test_cmp expect actual >> ' >> test_done >> diff --git a/t/t3700-add.sh b/t/t3700-add.sh >> index 51afbd7b24a..82dd768944f 100755 >> --- a/t/t3700-add.sh >> +++ b/t/t3700-add.sh >> @@ -106,24 +106,32 @@ test_expect_success '.gitignore test setup' ' >> test_expect_success '.gitignore is honored' ' >> git add . && >> - ! (git ls-files | grep "\\.ig") >> + git ls-files >files && >> + sed -n "/\\.ig/p" <files >actual && >> + test_must_be_empty actual >> ' >> test_expect_success 'error out when attempting to add ignored ones >> without -f' ' >> test_must_fail git add a.?? && >> - ! (git ls-files | grep "\\.ig") >> + git ls-files >files && >> + sed -n "/\\.ig/p" <files >actual && >> + test_must_be_empty actual >> ' >> test_expect_success 'error out when attempting to add ignored ones >> without -f' ' >> test_must_fail git add d.?? && >> - ! (git ls-files | grep "\\.ig") >> + git ls-files >files && >> + sed -n "/\\.ig/p" <files >actual && >> + test_must_be_empty actual >> ' >> test_expect_success 'error out when attempting to add ignored ones >> but add others' ' >> touch a.if && >> test_must_fail git add a.?? && >> - ! (git ls-files | grep "\\.ig") && >> - (git ls-files | grep a.if) >> + git ls-files >files && >> + sed -n "/\\.ig/p" <files >actual && >> + test_must_be_empty actual && >> + grep a.if files >> ' >> test_expect_success 'add ignored ones with -f' '
Phillip Wood <phillip.wood123@gmail.com> writes: > It seems strange to use sed here, you could just keep using grep and > check the output is empty if you don't want to use > test_expect_code. I am not sure what you mean by test_expect_code, but we can do the "! grep -e pattern file" to ensure that the unwanted pattern does not exist. Unlike our command, we do not worry about system "grep" being buggy, so if it yields non-zero, it is because no line in the file matches the pattern. After all, that is what the original code fixed by this patch wanted to check. The "do not lose exit code of the git command" part of the goal is achieved by breaking the pipeline already, and we can keep the test that uses grep. Having said that, switching to "sed" is not too bad. If we wanted to expect N lines that matches the pattern in the file, with grep, we need to do "! grep" dance when (and only when) N is zero. With sed, we do not have to worry about it. > There is also no need to redirect the input of the > sed commands. That's true.
diff --git a/t/t0055-beyond-symlinks.sh b/t/t0055-beyond-symlinks.sh index 6bada370225..c3eb1158ef9 100755 --- a/t/t0055-beyond-symlinks.sh +++ b/t/t0055-beyond-symlinks.sh @@ -15,12 +15,22 @@ test_expect_success SYMLINKS setup ' test_expect_success SYMLINKS 'update-index --add beyond symlinks' ' test_must_fail git update-index --add c/d && - ! ( git ls-files | grep c/d ) + cat >expect <<-\EOF && + a + b/d + EOF + git ls-files >actual && + test_cmp expect actual ' test_expect_success SYMLINKS 'add beyond symlinks' ' test_must_fail git add c/d && - ! ( git ls-files | grep c/d ) + cat >expect <<-\EOF && + a + b/d + EOF + git ls-files >actual && + test_cmp expect actual ' test_done diff --git a/t/t3700-add.sh b/t/t3700-add.sh index 51afbd7b24a..82dd768944f 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -106,24 +106,32 @@ test_expect_success '.gitignore test setup' ' test_expect_success '.gitignore is honored' ' git add . && - ! (git ls-files | grep "\\.ig") + git ls-files >files && + sed -n "/\\.ig/p" <files >actual && + test_must_be_empty actual ' test_expect_success 'error out when attempting to add ignored ones without -f' ' test_must_fail git add a.?? && - ! (git ls-files | grep "\\.ig") + git ls-files >files && + sed -n "/\\.ig/p" <files >actual && + test_must_be_empty actual ' test_expect_success 'error out when attempting to add ignored ones without -f' ' test_must_fail git add d.?? && - ! (git ls-files | grep "\\.ig") + git ls-files >files && + sed -n "/\\.ig/p" <files >actual && + test_must_be_empty actual ' test_expect_success 'error out when attempting to add ignored ones but add others' ' touch a.if && test_must_fail git add a.?? && - ! (git ls-files | grep "\\.ig") && - (git ls-files | grep a.if) + git ls-files >files && + sed -n "/\\.ig/p" <files >actual && + test_must_be_empty actual && + grep a.if files ' test_expect_success 'add ignored ones with -f' '
Change tests that would lose the "git" exit code via a negation pattern to: - In the case of "t0055-beyond-symlinks.sh" compare against the expected output instead. We could use the same pattern as in the "t3700-add.sh" below, doing so would have the advantage that if we added an earlier test we wouldn't need to adjust the "expect" output. But as "t0055-beyond-symlinks.sh" is a small and focused test (less than 40 lines in total) let's use "test_cmp" instead. - For "t3700-add.sh" use "sed -n" to print the expected "bad" part, and use "test_must_be_empty" to assert that it's not there. If we used "grep" we'd get a non-zero exit code. We could use "test_expect_code 1 grep", but this is more consistent with existing patterns in the test suite. We can also remove a repeated invocation of "git ls-files" for the last test that's being modified in that file, and search the existing "files" output instead. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t0055-beyond-symlinks.sh | 14 ++++++++++++-- t/t3700-add.sh | 18 +++++++++++++----- 2 files changed, 25 insertions(+), 7 deletions(-)