[FYI 6/7] {tap-test-protocols} testsuite: TAP tests properly decide when to remove tempdirs

[Stefano Lattarini]

Before this change, the TAP tests in the Automake testsuite were
removing the temporary test directory even when they failed or
were skipped, thus making debugging more difficult.

* tests/tap-functions.sh (incr_tap_count): Removed, superseded
by ...
(incr_): ... this function, which can increment the value of any
variable passed to it.
(result_):  Updated to use `incr_' instead of the now-removed
`incr_tap_count_'.  Keep count of failures, xfailures, xpasses,
and skips, using ...
($tap_skip_count_, $tap_bad_count, _$tap_xfail_count_): ...
these new variables.
* tests/defs (trap): Try to use their values to decide whether
the temporary directory being used by the test script should be
removed or not.  Other code reorganizations.  And move the code
for the removal of the temporary directory out to ...
(rm_rf_): ... this new subroutine.
(Main code): Use that instead of duplicating the code.
---
 ChangeLog              |   22 ++++++++++++++
 tests/defs             |   72 ++++++++++++++++++++++++++++++-----------------
 tests/tap-functions.sh |   22 ++++++++++++--
 3 files changed, 86 insertions(+), 30 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f181624..01ff2a5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,27 @@
 2011-08-07  Stefano Lattarini  <address@hidden>
 
+       testsuite: TAP tests properly decide when to remove tempdirs
+       Before this change, the TAP tests in the Automake testsuite were
+       removing the temporary test directory even when they failed or
+       were skipped, thus making debugging more difficult.
+       * tests/tap-functions.sh (incr_tap_count): Removed, superseded
+       by ...
+       (incr_): ... this function, which can increment the value of any
+       variable passed to it.
+       (result_):  Updated to use `incr_' instead of the now-removed
+       `incr_tap_count_'.  Keep count of failures, xfailures, xpasses,
+       and skips, using ...
+       ($tap_skip_count_, $tap_bad_count, _$tap_xfail_count_): ...
+       these new variables.
+       * tests/defs (trap): Try to use their values to decide whether
+       the temporary directory being used by the test script should be
+       removed or not.  Other code reorganizations.  And move the code
+       for the removal of the temporary directory out to ...
+       (rm_rf_): ... this new subroutine.
+       (Main code): Use that instead of duplicating the code.
+
+2011-08-07  Stefano Lattarini  <address@hidden>
+
        testsuite: improve and refactor our custom TAP shell library
        This change is mostly done in preparation for the next one.
        * tests/tap-functions.sh (result_with_directive_): Merge into ...
diff --git a/tests/defs b/tests/defs
index e9e241e..bfe7fa4 100644
--- a/tests/defs
+++ b/tests/defs
@@ -322,6 +322,20 @@ seq_ ()
   done
 }
 
+# rm_rf_ [FILES OR DIRECTORIES ...]
+# ---------------------------------
+# Recursively remove the given files or directory, also handling the case
+# of non-writable subdirectories.
+rm_rf_ ()
+{
+  test $# -gt 0 || return 0
+  # Ignore failures in find, we are only interested in failures of the
+  # final rm.
+  find "$@" -type d ! -perm -700 -exec chmod u+rwx {} \; || :
+  rm -rf "$@"
+}
+
+
 # count_test_results total=N pass=N fail=N xpass=N xfail=N skip=N error=N
 # -----------------------------------------------------------------------
 # Check that a testsuite run driven by the parallel-tests harness has
@@ -802,10 +816,7 @@ distdir=$me-1.0
 # this directory will be kept, to facilitate debugging.
 testSubDir=$me.dir
 
-test ! -d $testSubDir || {
-  find $testSubDir -type d ! -perm -700 -exec chmod u+rwx {} ";"
-  rm -rf $testSubDir
-}
+test ! -d $testSubDir || rm_rf_ $testSubDir
 mkdir $testSubDir
 
 cd ./$testSubDir
@@ -814,31 +825,40 @@ if test "$sh_errexit_works" = yes; then
   trap 'exit_status=$?
     set +e
     cd "$testbuilddir"
-    # This is to ensure that a test script does give a SKIP outcome just
-    # because a command in it happens to exit with status 77.  This
-    # behaviour, while from time to time useful to developers, is not
-    # meant to be enabled by default, as it could cause spurious failures
-    # in the wild.  Thus it will be enabled only when the variable
-    # "am_explicit_skips" is set to a "true" value.
-    case $am_explicit_skips in
-      [yY]|[yY]es|1)
-        if test $exit_status -eq 77 && test $am__test_skipped != yes; then
-          exit_status=78
-        fi
-        ;;
-    esac
-    case $exit_status,$keep_testdirs in
-    0,)
-      find $testSubDir -type d ! -perm -700 -exec chmod u+rwx {} ";"
-      rm -rf $testSubDir
-      ;;
-    esac
+    if test -n "$keep_testdirs"; then
+      keep_testdirs=yes
+    else
+      keep_testdirs=no
+    fi
+    if test $using_tap = yes; then
+      if test $have_tap_plan_ != yes; then
+        late_plan_
+      fi
+      test $exit_status -eq 0 \
+        && test $tap_xfail_count_ -eq 0 \
+        && test $tap_skip_count_ -eq 0 \
+        && test $tap_bad_count_ -eq 0 \
+        || keep_testdirs=yes
+    else
+      # This is to ensure that a test script does give a SKIP outcome just
+      # because a command in it happens to exit with status 77.  This
+      # behaviour, while from time to time useful to developers, is not
+      # meant to be enabled by default, as it could cause spurious failures
+      # in the wild.  Thus it will be enabled only when the variable
+      # "am_explicit_skips" is set to a "true" value.
+      case $am_explicit_skips in
+        [yY]|[yY]es|1)
+          if test $exit_status -eq 77 && test $am__test_skipped != yes; then
+            echo "$me: implicit skip turned into failure"
+            exit_status=78
+          fi;;
+      esac
+      test $exit_status -eq 0 || keep_testdirs=yes
+    fi
+    test $keep_testdirs = no && rm_rf_ $testSubDir
     set +x
     test "$signal" != 0 && echo "$me: caught signal $signal"
     echo "$me: exit $exit_status"
-    if test $using_tap = yes && test $have_tap_plan_ != yes; then
-      late_plan_
-    fi
     exit $exit_status
   ' 0
   for signal in 1 2 13 15; do
diff --git a/tests/tap-functions.sh b/tests/tap-functions.sh
index b638e6f..d685c18 100644
--- a/tests/tap-functions.sh
+++ b/tests/tap-functions.sh
@@ -24,15 +24,22 @@
 
 # The count of the TAP test results seen so far.
 tap_count_=0
+# The count of skipped tests.
+tap_skip_count_=0
+# The count of tests that experienced an expected failure.
+tap_xfail_count_=0
+# The count of tests with unexpected outcomes (i.e., failed and xpassed).
+tap_bad_count_=0
 
 # The first "test -n" tries to avoid extra forks when possible.
 if test -n "${ZSH_VERSION}${BASH_VERSION}" \
      || (eval 'test $((1 + 1)) = 2') >/dev/null 2>&1
 then
-  # Use of 'eval' needed to protect dumber shells from parsing errors.
-  eval 'incr_tap_count_ () { tap_count_=$(($tap_count_ + 1)); }'
+  # Outer use of 'eval' needed to protect dumber shells from parsing
+  # errors.
+  eval 'incr_ () { eval "$1=\$((\${$1} + 1))"; }'
 else
-  incr_tap_count_ () { tap_count_=`expr $tap_count_ + 1`; }
+  incr_ () { eval "$1=\`expr \${$1} + 1\`"; }
 fi
 
 # plan_ NUMBER-OF-PLANNED-TESTS
@@ -113,7 +120,14 @@ result_ ()
     ""|TODO|SKIP) ;;
     *) bailout_ "result_: invalid directive '$directive_'" ;;
   esac
-  incr_tap_count_
+  incr_ tap_count_
+  case $tap_result_,$tap_directive_ in
+    ok,) ;;                                     # Passed.
+    not\ ok,TODO) incr_ tap_xfail_count_;;      # Expected failure.
+    not\ ok,*|ok,TODO) incr_ tap_bad_count_ ;;  # Failed or xpassed.
+    ok,SKIP) incr_ tap_skip_count_ ;;           # Skipped.
+    *) bailout_ "internal error in 'result_'";; # Can't happen.
+  esac
   tap_text_="$tap_result_ $tap_count_"
   if test x"$*" != x; then
     tap_text_="$tap_text_ - $*"
-- 
1.7.2.3