[FYI] {test-protocols} tap: scripts with a SKIP plan but with exit status != 0 must error

[Stefano Lattarini]

This change has been motivated by Automake's own testsuite.  Some
TAP tests there were erroring out (due to unexpected, unhandled
failures) before having encountered TAP result, so that the
simple-minded shell library implementing TAP generation ended up
printing a "1..0" trailing test plan; this caused the script to be
reported as a SKIP rather than an ERROR -- a nasty false negative.

* lib/tap-driver: Add prototypes for each subroutine, to free up
the order in which they can be defined and called.
(main): Move the code checking for a bad exit status of the TAP
producer ...
(finish): ... here, and flush the TAP stream to ensure that the
parser always obtains the producer's exit status.
* tests/tap-skip-whole-badexit.test: New test.
* tests/Makefile.am (tap_with_common_setup_tests): Add it.
---
 ChangeLog                         |   18 +++++++++++
 lib/tap-driver                    |   61 ++++++++++++++++++++++++++++---------
 tests/Makefile.am                 |    1 +
 tests/Makefile.in                 |    1 +
 tests/tap-skip-whole-badexit.test |   52 +++++++++++++++++++++++++++++++
 5 files changed, 118 insertions(+), 15 deletions(-)
 create mode 100755 tests/tap-skip-whole-badexit.test

diff --git a/ChangeLog b/ChangeLog
index e07ef47..75ea7a2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,23 @@
 2011-08-06  Stefano Lattarini  <address@hidden>
 
+       tap: scripts with a SKIP plan but with exit status != 0 must error
+       This change has been motivated by Automake's own testsuite.  Some
+       TAP tests there were erroring out (due to unexpected, unhandled
+       failures) before having encountered TAP result, so that the
+       simple-minded shell library implementing TAP generation ended up
+       printing a "1..0" trailing test plan; this caused the script to be
+       reported as a SKIP rather than an ERROR -- a nasty false negative.
+       * lib/tap-driver: Add prototypes for each subroutine, to free up
+       the order in which they can be defined and called.
+       (main): Move the code checking for a bad exit status of the TAP
+       producer ...
+       (finish): ... here, and flush the TAP stream to ensure that the
+       parser always obtains the producer's exit status.
+       * tests/tap-skip-whole-badexit.test: New test.
+       * tests/Makefile.am (tap_with_common_setup_tests): Add it.
+
+2011-08-06  Stefano Lattarini  <address@hidden>
+
        tap: fix whitespace munging of diagnostic messages
        * lib/tap-driver (extract_tap_comment): Pass the `g' flag to the
        substitution operator, to strip also trailing whitespaces.  Fixes
diff --git a/lib/tap-driver b/lib/tap-driver
index d59ae01..d836b5f 100755
--- a/lib/tap-driver
+++ b/lib/tap-driver
@@ -83,6 +83,34 @@ Getopt::Long::GetOptions (
     'ignore-exit' => sub { $cfg{"ignore-exit"} = 1; },
   ) or exit 1;
 
+# ------------- #
+#  Prototypes.  #
+# ------------- #
+
+sub add_test_result ($);
+sub bool_opt ($$);
+sub colored ($$);
+sub copy_in_global_log ();
+sub decorate_result ($);
+sub extract_tap_comment ($);
+sub finish ();
+sub get_global_test_result ();
+sub get_tap_line ();
+sub get_test_results ();
+sub handle_tap_bailout ($);
+sub handle_tap_plan ($);
+sub handle_tap_test ($);
+sub main (@);
+sub must_recheck ();
+sub peek_tap_line ();
+sub report ($;$);
+sub start (@);
+sub stringify_test_result ($);
+sub testuite_error ($);
+sub unget_tap_line ($);
+sub write_test_results ();
+sub yn ($);
+
 # -------------- #
 #  Subroutines.  #
 # -------------- #
@@ -204,6 +232,24 @@ sub start (@)
 
 sub finish ()
 {
+  # Flush all the remaining TAP stream, so that we can obtain the
+  # exit status of the TAP producer.
+  do {} while defined get_tap_line ();
+  # TODO: we should probably use $parser->wait here, to catch signals too
+  if ($parser->exit != 0)
+    {
+      my $msg = sprintf "exited with status %d", $parser->exit;
+      if ($cfg{"ignore-exit"})
+        {
+          # Log the exit status of the script anyway, even if it is not
+          # considered to be an error, to help debugging.
+          print "INFO: $test_script_name - $msg\n";
+        }
+      else
+        {
+          testuite_error $msg;
+        }
+    }
   write_test_results;
   close LOG or die "closing $log_file: $!\n";
   exit 0;
@@ -400,21 +446,6 @@ sub main (@)
       testuite_error (sprintf "too %s tests run (expected %d, got %d)",
                               $bad_amount, $planned, $run);
     }
-  # TODO: we should probably use $parser->wait here, to catch signals too
-  if ($parser->exit != 0)
-  {
-    my $msg = sprintf "exited with status %d", $parser->exit;
-    if ($cfg{"ignore-exit"})
-      {
-        # Log the exit status of the script anyway, even if it is not
-        # considered to be an error, to help debugging.
-        print "INFO: $test_script_name - $msg\n";
-      }
-    else
-      {
-        testuite_error $msg;;
-      }
-  }
   finish;
 }
 
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 7b3b777..4e7cbda 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1177,6 +1177,7 @@ tap-recheck-logs.test \
 tap-skip-whole-whitespace.test \
 tap-skip-whole.test \
 tap-skip-whole-lastline.test \
+tap-skip-whole-badexit.test \
 tap-todo-skip-together.test \
 tap-todo-skip-whitespace.test \
 tap-todo-skip.test \
diff --git a/tests/Makefile.in b/tests/Makefile.in
index f03ec16..12ba08f 100644
--- a/tests/Makefile.in
+++ b/tests/Makefile.in
@@ -1413,6 +1413,7 @@ tap-recheck-logs.test \
 tap-skip-whole-whitespace.test \
 tap-skip-whole.test \
 tap-skip-whole-lastline.test \
+tap-skip-whole-badexit.test \
 tap-todo-skip-together.test \
 tap-todo-skip-whitespace.test \
 tap-todo-skip.test \
diff --git a/tests/tap-skip-whole-badexit.test 
b/tests/tap-skip-whole-badexit.test
new file mode 100755
index 0000000..76ba692
--- /dev/null
+++ b/tests/tap-skip-whole-badexit.test
@@ -0,0 +1,52 @@
+#! /bin/sh
+# Copyright (C) 2011 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2, or (at your option)
+# any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# TAP support:
+#  - an exit status != 0 of a test script causes an hard error, even if
+#    the last line of output is a "SKIP plan" (e.g., "1..0 # SKIP").
+
+parallel_tests=yes
+. ./defs || Exit 1
+
+echo TESTS = one.test two.test > Makefile.am
+
+. "$testsrcdir"/tap-setup.sh || fatal_ "sourcing tap-setup.sh"
+
+cat > one.test <<'END'
+#!/bin/sh
+echo '1..0 # SKIP'
+exit 1
+END
+
+cat > two.test <<'END'
+#!/bin/sh
+echo '1..0'
+exit 22
+END
+
+chmod a+x one.test two.test
+
+$MAKE check >stdout && { cat stdout; Exit 1; }
+cat stdout
+
+# The 'prove' utility reports both the skip and the non-zero exit status,
+# so we do the same.
+count_test_results total=4 pass=0 fail=0 xpass=0 xfail=0 skip=2 error=2
+
+grep '^ERROR: one\.test - exited with status 1$' stdout
+grep '^ERROR: two\.test - exited with status 22$' stdout
+
+:
-- 
1.7.2.3