| Message ID | patch-v4-1.6-68d276dd421-20221219T101240Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fb18dd2831323d5dece711cdbcaa9dcd8bf4edab |
| Headers | show |
| Series | tests: fix ignored & hidden exit codes | expand |
Am 19.12.22 um 11:19 schrieb Ævar Arnfjörð Bjarmason: > Change the functions which are called from within > "test_expect_success" to add the "|| return 1" idiom to their > for-loops, so we won't lose the exit code of "cp", "git" etc. > > Then for those setup functions that aren't called from a > "test_expect_success" we need to put the setup code in a > "test_expect_success" as well. It would not be enough to properly > &&-chain these, as the calling code is the top-level script itself. As > we don't run the tests with "set -e" we won't report failing commands > at the top-level. > > The "checkout" part of this would miss memory leaks under > SANITIZE=leak, this code doesn't leak (the relevant "git checkout" > leak has been fixed), but in a past version of git we'd continue past > this failure under SANITIZE=leak when these invocations had errored > out, even under "--immediate". > > Helped-by: René Scharfe <l.s.r@web.de> > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/t0027-auto-crlf.sh | 66 +++++++++++++++++++++++++------------------- > 1 file changed, 38 insertions(+), 28 deletions(-) > > diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh > index a94ac1eae37..2f57c8669cb 100755 > --- a/t/t0027-auto-crlf.sh > +++ b/t/t0027-auto-crlf.sh > @@ -70,7 +70,8 @@ create_NNO_MIX_files () { > cp CRLF ${pfx}_CRLF.txt && > cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt && > cp LF_mix_CR ${pfx}_LF_mix_CR.txt && > - cp CRLF_nul ${pfx}_CRLF_nul.txt > + cp CRLF_nul ${pfx}_CRLF_nul.txt || > + return 1 > done > done > done > @@ -101,7 +102,8 @@ commit_check_warn () { > do > fname=${pfx}_$f.txt && > cp $f $fname && > - git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" > + git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" || > + return 1 > done && > git commit -m "core.autocrlf $crlf" && > check_warning "$lfname" ${pfx}_LF.err && > @@ -121,15 +123,19 @@ commit_chk_wrnNNO () { > lfmixcr=$1 ; shift > crlfnul=$1 ; shift > pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf} > - #Commit files on top of existing file > - create_gitattributes "$attr" $aeol && > - for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul > - do > - fname=${pfx}_$f.txt && > - cp $f $fname && > - printf Z >>"$fname" && > - git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" > - done > + > + test_expect_success 'setup commit NNO files' ' > + #Commit files on top of existing file > + create_gitattributes "$attr" $aeol && > + for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul > + do > + fname=${pfx}_$f.txt && > + cp $f $fname && > + printf Z >>"$fname" && > + git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" || > + return 1 > + done > + ' > > test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" ' > check_warning "$lfwarn" ${pfx}_LF.err > @@ -163,15 +169,19 @@ commit_MIX_chkwrn () { > lfmixcr=$1 ; shift > crlfnul=$1 ; shift > pfx=MIX_attr_${attr}_aeol_${aeol}_${crlf} > - #Commit file with CLRF_mix_LF on top of existing file > - create_gitattributes "$attr" $aeol && > - for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul > - do > - fname=${pfx}_$f.txt && > - cp CRLF_mix_LF $fname && > - printf Z >>"$fname" && > - git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" > - done > + > + test_expect_success 'setup commit file with mixed EOL' ' > + #Commit file with CLRF_mix_LF on top of existing file > + create_gitattributes "$attr" $aeol && > + for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul > + do > + fname=${pfx}_$f.txt && > + cp CRLF_mix_LF $fname && > + printf Z >>"$fname" && > + git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" || > + return 1 > + done > + ' > > test_expect_success "commit file with mixed EOL onto LF crlf=$crlf attr=$attr" ' > check_warning "$lfwarn" ${pfx}_LF.err > @@ -289,17 +299,17 @@ checkout_files () { > lfmixcrlf=$1 ; shift > lfmixcr=$1 ; shift > crlfnul=$1 ; shift > - create_gitattributes "$attr" $ident $aeol && > - git config core.autocrlf $crlf && > + test_expect_success "setup config for checkout attr=$attr ident=$ident aeol=$aeol core.autocrlf=$crlf" ' > + create_gitattributes "$attr" $ident $aeol && > + git config core.autocrlf $crlf > + ' > pfx=eol_${ceol}_crlf_${crlf}_attr_${attr}_ && > for f in LF CRLF LF_mix_CR CRLF_mix_LF LF_nul > do > - rm crlf_false_attr__$f.txt && > - if test -z "$ceol"; then > - git checkout -- crlf_false_attr__$f.txt > - else > - git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt > - fi > + test_expect_success "setup $f checkout ${ceol:+ with -c core.eol=$ceol}" ' > + rm -f crlf_false_attr__$f.txt && > + git ${ceol:+-c core.eol=$ceol} checkout -- crlf_false_attr__$f.txt > + ' OK, but why have test_expect_success inside the loop here, while a simlilar loop is inside the two test_expect_success bodies above? Do we really need five invocations here instead of one? It's just setup code. > done > > test_expect_success "ls-files --eol attr=$attr $ident aeol=$aeol core.autocrlf=$crlf core.eol=$ceol" '
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh index a94ac1eae37..2f57c8669cb 100755 --- a/t/t0027-auto-crlf.sh +++ b/t/t0027-auto-crlf.sh @@ -70,7 +70,8 @@ create_NNO_MIX_files () { cp CRLF ${pfx}_CRLF.txt && cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt && cp LF_mix_CR ${pfx}_LF_mix_CR.txt && - cp CRLF_nul ${pfx}_CRLF_nul.txt + cp CRLF_nul ${pfx}_CRLF_nul.txt || + return 1 done done done @@ -101,7 +102,8 @@ commit_check_warn () { do fname=${pfx}_$f.txt && cp $f $fname && - git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" + git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" || + return 1 done && git commit -m "core.autocrlf $crlf" && check_warning "$lfname" ${pfx}_LF.err && @@ -121,15 +123,19 @@ commit_chk_wrnNNO () { lfmixcr=$1 ; shift crlfnul=$1 ; shift pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf} - #Commit files on top of existing file - create_gitattributes "$attr" $aeol && - for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul - do - fname=${pfx}_$f.txt && - cp $f $fname && - printf Z >>"$fname" && - git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" - done + + test_expect_success 'setup commit NNO files' ' + #Commit files on top of existing file + create_gitattributes "$attr" $aeol && + for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul + do + fname=${pfx}_$f.txt && + cp $f $fname && + printf Z >>"$fname" && + git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" || + return 1 + done + ' test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" ' check_warning "$lfwarn" ${pfx}_LF.err @@ -163,15 +169,19 @@ commit_MIX_chkwrn () { lfmixcr=$1 ; shift crlfnul=$1 ; shift pfx=MIX_attr_${attr}_aeol_${aeol}_${crlf} - #Commit file with CLRF_mix_LF on top of existing file - create_gitattributes "$attr" $aeol && - for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul - do - fname=${pfx}_$f.txt && - cp CRLF_mix_LF $fname && - printf Z >>"$fname" && - git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" - done + + test_expect_success 'setup commit file with mixed EOL' ' + #Commit file with CLRF_mix_LF on top of existing file + create_gitattributes "$attr" $aeol && + for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul + do + fname=${pfx}_$f.txt && + cp CRLF_mix_LF $fname && + printf Z >>"$fname" && + git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err" || + return 1 + done + ' test_expect_success "commit file with mixed EOL onto LF crlf=$crlf attr=$attr" ' check_warning "$lfwarn" ${pfx}_LF.err @@ -289,17 +299,17 @@ checkout_files () { lfmixcrlf=$1 ; shift lfmixcr=$1 ; shift crlfnul=$1 ; shift - create_gitattributes "$attr" $ident $aeol && - git config core.autocrlf $crlf && + test_expect_success "setup config for checkout attr=$attr ident=$ident aeol=$aeol core.autocrlf=$crlf" ' + create_gitattributes "$attr" $ident $aeol && + git config core.autocrlf $crlf + ' pfx=eol_${ceol}_crlf_${crlf}_attr_${attr}_ && for f in LF CRLF LF_mix_CR CRLF_mix_LF LF_nul do - rm crlf_false_attr__$f.txt && - if test -z "$ceol"; then - git checkout -- crlf_false_attr__$f.txt - else - git -c core.eol=$ceol checkout -- crlf_false_attr__$f.txt - fi + test_expect_success "setup $f checkout ${ceol:+ with -c core.eol=$ceol}" ' + rm -f crlf_false_attr__$f.txt && + git ${ceol:+-c core.eol=$ceol} checkout -- crlf_false_attr__$f.txt + ' done test_expect_success "ls-files --eol attr=$attr $ident aeol=$aeol core.autocrlf=$crlf core.eol=$ceol" '
Change the functions which are called from within "test_expect_success" to add the "|| return 1" idiom to their for-loops, so we won't lose the exit code of "cp", "git" etc. Then for those setup functions that aren't called from a "test_expect_success" we need to put the setup code in a "test_expect_success" as well. It would not be enough to properly &&-chain these, as the calling code is the top-level script itself. As we don't run the tests with "set -e" we won't report failing commands at the top-level. The "checkout" part of this would miss memory leaks under SANITIZE=leak, this code doesn't leak (the relevant "git checkout" leak has been fixed), but in a past version of git we'd continue past this failure under SANITIZE=leak when these invocations had errored out, even under "--immediate". Helped-by: René Scharfe <l.s.r@web.de> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t0027-auto-crlf.sh | 66 +++++++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 28 deletions(-)