[FYI 1/2] {test-protocols} tap: a "plan with SKIP" after test results is an error

[Stefano Lattarini]

This new behaviour is both more natural and more consistent with
that of the `prove' utility.

* lib/tap-driver (handle_tap_plan): Do not stop TAP parsing when
a "plan with SKIP" line (e.g., "1..0 # SKIP") is encountered, and
do not print a "SKIP" line if some TAP result has already been
seen.
* tests/tap-skip-whole.test: Adapt.
* tests/tap-skip-whole-lastline.test: Likewise.
* tests/tap-global-result.test: Adapt and extend.
* tests/tap-skip-plan-errors.test: Likewise, and fix an obsolete
small part of a comment.
* tests/tap-skip-whole-bailout.test: New test.
* tests/tap-skip-whole-unplanned.test: Likewise.
* tests/tap-skip-whole-continue.test: Likewise.
* tests/Makefile.am (tap_with_common_setup_tests): Update.
---
 ChangeLog                           |   19 ++++++++
 lib/tap-driver                      |   21 +++++---
 tests/Makefile.am                   |    3 +
 tests/Makefile.in                   |    3 +
 tests/tap-global-result.test        |   85 +++++++++++++++++++++++++++++++----
 tests/tap-plan-corner.test          |   28 +++++++++++
 tests/tap-plan-errors.test          |   31 +++++++++++--
 tests/tap-skip-whole-bailout.test   |   36 +++++++++++++++
 tests/tap-skip-whole-continue.test  |   63 ++++++++++++++++++++++++++
 tests/tap-skip-whole-lastline.test  |   14 ++----
 tests/tap-skip-whole-unplanned.test |   73 ++++++++++++++++++++++++++++++
 tests/tap-skip-whole.test           |   23 +++-------
 12 files changed, 351 insertions(+), 48 deletions(-)
 create mode 100755 tests/tap-skip-whole-bailout.test
 create mode 100755 tests/tap-skip-whole-continue.test
 create mode 100755 tests/tap-skip-whole-unplanned.test

diff --git a/ChangeLog b/ChangeLog
index 4e50d37..4ea0ee7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,22 @@
+2011-08-11  Stefano Lattarini  <address@hidden>
+
+       tap: a "plan with SKIP" after test results is an error
+       This new behaviour is both more natural and more consistent with
+       that of the `prove' utility.
+       * lib/tap-driver (handle_tap_plan): Do not stop TAP parsing when
+       a "plan with SKIP" line (e.g., "1..0 # SKIP") is encountered, and
+       do not print a "SKIP" line if some TAP result has already been
+       seen.
+       * tests/tap-skip-whole.test: Adapt.
+       * tests/tap-skip-whole-lastline.test: Likewise.
+       * tests/tap-global-result.test: Adapt and extend.
+       * tests/tap-skip-plan-errors.test: Likewise, and fix an obsolete
+       small part of a comment.
+       * tests/tap-skip-whole-bailout.test: New test.
+       * tests/tap-skip-whole-unplanned.test: Likewise.
+       * tests/tap-skip-whole-continue.test: Likewise.
+       * tests/Makefile.am (tap_with_common_setup_tests): Update.
+
 2011-08-09  Stefano Lattarini  <address@hidden>
 
        test harness: avoid possible fork bomb
diff --git a/lib/tap-driver b/lib/tap-driver
index 2393346..a02557d 100755
--- a/lib/tap-driver
+++ b/lib/tap-driver
@@ -369,18 +369,23 @@ sub handle_tap_plan ($)
   my $plan = shift;
   # Only one plan per stream is acceptable.
   testsuite_error "multiple test plans" if $plan_seen;
-  $plan_seen = 1;
   # TAP plan must come either before or after *all* the TAP results.
   # So, if we find it after having already seen at least one TAP result,
   # set a flag signaling that no more TAP results are acceptable.
   $tap_stopped = 1 if $testno >= 1;
-  # Nothing more to do, unless the plan contains a SKIP directive.
-  return
-    if not defined $plan->directive && length ($plan->directive) > 0;
-  my $explanation = $plan->explanation ?
-                    "- " . $plan->explanation : undef;
-  report "SKIP", $explanation;
-  finish;
+  # If the plan contains a SKIP directive, and it's not an error, we
+  # want to report it as a particular kind of SKIP result.
+  # If "$testno > 0", we have is an error that will be automatically
+  # dealt with later, so don't worry about it here.
+  # If "$plan_seen" is true, we have an error due to a repeated plan,
+  # and that has already been dealt with above.
+  if ($plan->directive && $testno == 0 && !$plan_seen)
+    {
+      my $explanation = $plan->explanation ?
+                        "- " . $plan->explanation : undef;
+      report "SKIP", $explanation;
+    }
+  $plan_seen = 1;
 }
 
 sub handle_tap_bailout ($)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index bac1d32..ae76516 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1179,6 +1179,9 @@ tap-skip-whole-whitespace.test \
 tap-skip-whole.test \
 tap-skip-whole-lastline.test \
 tap-skip-whole-badexit.test \
+tap-skip-whole-bailout.test \
+tap-skip-whole-continue.test \
+tap-skip-whole-unplanned.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 9e03806..d43abe9 100644
--- a/tests/Makefile.in
+++ b/tests/Makefile.in
@@ -1419,6 +1419,9 @@ tap-skip-whole-whitespace.test \
 tap-skip-whole.test \
 tap-skip-whole-lastline.test \
 tap-skip-whole-badexit.test \
+tap-skip-whole-bailout.test \
+tap-skip-whole-continue.test \
+tap-skip-whole-unplanned.test \
 tap-todo-skip-together.test \
 tap-todo-skip-whitespace.test \
 tap-todo-skip.test \
diff --git a/tests/tap-global-result.test b/tests/tap-global-result.test
index 32e2401..7b83e6a 100755
--- a/tests/tap-global-result.test
+++ b/tests/tap-global-result.test
@@ -39,7 +39,8 @@ END
 
 cat > skipall.test <<'END'
 1..0 # SKIP
-not ok 1
+foo
+# bar
 END
 
 cat > fail.test <<'END'
@@ -56,8 +57,70 @@ END
 
 (sed '1s/.*/1..4/' ok.test && echo 'ok 4 # TODO') > xpass2.test
 
-echo 'Bail out!' > error.test
-(cat ok.test && echo 'Bail out!') > error2.test
+echo 'Bail out!' > bail.test
+
+(cat ok.test && echo 'Bail out!') > bail2.test
+
+cat > bail3.test <<'END'
+1..0 # SKIP
+Bail out!
+END
+
+# Too many tests.
+cat > error.test <<'END'
+1..2
+ok 1
+ok 2 # SKIP
+not ok 3
+not ok 4 # TODO
+END
+
+# Too few tests.
+cat > error2.test <<'END'
+1..4
+ok 1
+not ok 2 # TODO
+ok 3 # SKIP
+END
+
+# Repeated plan.
+cat > error3.test <<'END'
+1..2
+1..2
+ok 1
+ok 2
+END
+
+# Too many tests, after a "SKIP" plan.
+cat > error4.test <<'END'
+1..0 # SKIP
+ok 1
+ok 2
+END
+
+# Tests out of order.
+cat > error5.test <<'END'
+1..4
+not ok 1 # TODO
+ok 3
+ok 2
+ok 4
+END
+
+# Wrong test number.
+cat > error6.test <<'END'
+1..2
+ok 1 # SKIP
+ok 7
+END
+
+# No plan.
+cat > error7.test <<'END'
+ok 1 # SKIP
+ok 2 # TODO
+not ok 3 # TODO
+ok 4
+END
 
 cat > hodgepodge.test <<'END'
 1..2
@@ -76,17 +139,13 @@ ok 4 # TODO
 Bail out!
 END
 
-# TODO: add scripts with TAP errors (multiple plans, out-of-order
-# tests, etc).
-
 tests=`echo *.test` # Also required later.
 
 TESTS="$tests" $MAKE -e check >stdout && { cat stdout; Exit 1; }
 cat stdout
 
-# Dirty tricks required here.
-for tst in ok skip skipall fail fail2 xpass xpass2 error error2 \
-           hodgepodge hodgepodge-all; do
+# Dirty trick required here.
+for tst in `echo " $tests " | sed 's/.test / /'`; do
   echo :copy-in-global-log: yes >> $tst.trs
 done
 
@@ -112,8 +171,16 @@ have_rst_section 'FAIL: fail'
 have_rst_section 'FAIL: fail2'
 have_rst_section 'FAIL: xpass'
 have_rst_section 'FAIL: xpass2'
+have_rst_section 'ERROR: bail'
+have_rst_section 'ERROR: bail2'
+have_rst_section 'ERROR: bail3'
 have_rst_section 'ERROR: error'
 have_rst_section 'ERROR: error2'
+have_rst_section 'ERROR: error3'
+have_rst_section 'ERROR: error4'
+have_rst_section 'ERROR: error5'
+have_rst_section 'ERROR: error6'
+have_rst_section 'ERROR: error7'
 have_rst_section 'ERROR: hodgepodge'
 have_rst_section 'ERROR: hodgepodge-all'
 
diff --git a/tests/tap-plan-corner.test b/tests/tap-plan-corner.test
index eaebcf2..a7c9933 100755
--- a/tests/tap-plan-corner.test
+++ b/tests/tap-plan-corner.test
@@ -65,6 +65,34 @@ for pos in leading trailing; do
   grep "^ERROR: $pos-repeated\\.test 2 # AFTER LATE PLAN$" stdout
 done
 
+cat > 1.test <<END
+1..0
+1..0
+END
+
+cat > 2.test <<END
+1..0 # SKIP
+1..0
+END
+
+cat > 3.test <<END
+1..0
+1..0 # SKIP
+END
+
+cat > 4.test <<END
+1..0 # SKIP
+1..0 # SKIP
+END
+
+env TESTS="1.test 2.test 3.test 4.test" \
+  $MAKE -e check >stdout && { cat stdout; Exit 1; }
+cat stdout
+count_test_results total=8 pass=0 fail=0 xpass=0 xfail=0 skip=4 error=4
+for i in 1 2 3 4; do
+  grep "^ERROR: $i\\.test - multiple test plans$" stdout
+done
+
 cat > all.test <<END
 1..5
 ok 1
diff --git a/tests/tap-plan-errors.test b/tests/tap-plan-errors.test
index 95cc640..9d8abcd 100755
--- a/tests/tap-plan-errors.test
+++ b/tests/tap-plan-errors.test
@@ -19,9 +19,9 @@
 #  - multiple test plans
 #  - missing test plan
 #  - misplaced test plan (tests run after a late plan)
-# Checks about unplanned tests are performed in 'tap-unplanned.test'.
-# More checks about corner-cases in TAP plans are performed in
-# 'tap-plan-corner.test' and 'tap-plan-corner2.test'.
+# Checks about unplanned tests are performed in 'tap-unplanned.test'
+# and 'tap-skip-whole-badcount.test'.  More checks about corner-cases
+# in TAP plans are performed in 'tap-plan-corner.test'.
 
 parallel_tests=yes
 . ./defs || Exit 1
@@ -76,6 +76,12 @@ ok 4
 ok 5
 END
 
+err='- missing test plan'
+my_check total=2 pass=1 fail=0 xpass=0 xfail=0 skip=0 error=1 <<END
+ok 1
+END
+
+
 # The two test plans here are deliberately equal.
 err='- multiple test plans'
 my_check total=3 pass=2 fail=0 xpass=0 xfail=0 skip=0 error=1 <<END
@@ -85,9 +91,24 @@ ok 2
 1..2
 END
 
-err='- missing test plan'
-my_check total=2 pass=1 fail=0 xpass=0 xfail=0 skip=0 error=1 <<END
+# The second plan is diagnosed as extra, and only the first one is
+# relevant w.r.t. the number of the expected test.
+err='- multiple test plans'
+my_check total=4 pass=3 fail=0 xpass=0 xfail=0 skip=0 error=1 <<END
+1..3
 ok 1
+ok 2
+1..2
+ok 3
+END
+
+# As above, in a slightly different situation.
+err='- multiple test plans'
+my_check total=3 pass=2 fail=0 xpass=0 xfail=0 skip=0 error=1 <<END
+1..2
+ok 1
+ok 2
+1..4
 END
 
 :
diff --git a/tests/tap-skip-whole-bailout.test 
b/tests/tap-skip-whole-bailout.test
new file mode 100755
index 0000000..0048e00
--- /dev/null
+++ b/tests/tap-skip-whole-bailout.test
@@ -0,0 +1,36 @@
+#! /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:
+#  - a "Bail out!" directive causes an hard error, even if coming after
+#    a "SKIP plan" (e.g., "1..0 # SKIP").
+
+parallel_tests=yes
+. ./defs || Exit 1
+
+. "$testsrcdir"/tap-setup.sh || fatal_ "sourcing tap-setup.sh"
+
+cat > all.test <<END
+1..0 # SKIP
+Bail out!
+END
+
+$MAKE check >stdout && { cat stdout; Exit 1; }
+cat stdout
+count_test_results total=2 pass=0 fail=0 xpass=0 xfail=0 skip=1 error=1
+grep '^ERROR: all\.test - Bail out!' stdout
+
+:
diff --git a/tests/tap-skip-whole-continue.test 
b/tests/tap-skip-whole-continue.test
new file mode 100755
index 0000000..274fe54
--- /dev/null
+++ b/tests/tap-skip-whole-continue.test
@@ -0,0 +1,63 @@
+#! /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/>.
+
+# Basic TAP test protocol support:
+#  - parsing of input TAP stream continue after a "SKIP" plan has
+#    been seen.
+
+parallel_tests=yes
+. ./defs || Exit 1
+
+. "$testsrcdir"/tap-setup.sh || fatal_ "sourcing tap-setup.sh"
+
+echo TEST_LOG_DRIVER_FLAGS = --comments >> Makefile
+
+cat > foo.test <<END
+1..0
+a non-TAP line
+# a comment
+END
+
+cat > bar.test <<END
+# an early comment
+an early non-TAP line
+ $tab
+1..0 # SKIP
+# a later comment
+a later non-TAP line
+END
+
+TESTS='foo.test bar.test' $MAKE -e check >stdout || { cat stdout; Exit 1; }
+cat stdout
+
+count_test_results total=2 pass=0 fail=0 xpass=0 xfail=0 skip=2 error=0
+
+grep '^# foo\.test: a comment$' stdout
+grep '^# bar\.test: an early comment$' stdout
+grep '^# bar\.test: a later comment$' stdout
+
+cat foo.log
+cat bar.log
+
+grep '^a non-TAP line$' foo.log
+grep '^# a comment$' foo.log
+grep '^# an early comment' bar.log
+grep '^an early non-TAP line$' bar.log
+grep '^# a later comment' bar.log
+grep '^a later non-TAP line$' bar.log
+grep "^ $tab$" bar.log
+
+:
diff --git a/tests/tap-skip-whole-lastline.test 
b/tests/tap-skip-whole-lastline.test
index 25697e0..56bb4e2 100755
--- a/tests/tap-skip-whole-lastline.test
+++ b/tests/tap-skip-whole-lastline.test
@@ -36,24 +36,18 @@ cat > bar.test <<END
 1..0
 END
 
-cat > baz.test <<END
-ok 1 - seen also if the whole test is skipped!
-1..0 # SKIP
-END
-
 st=0
-TESTS='foo.test bar.test baz.test' $MAKE -e check >stdout || st=$?
+TESTS='foo.test bar.test' $MAKE -e check >stdout || st=$?
 cat stdout
 cat test-suite.log
 test $st -eq 0
 
 grep '^SKIP: foo\.test .* from the last line$' stdout
 grep '^SKIP: bar\.test$' stdout
-grep '^SKIP: baz\.test$' stdout
 grep '^# bar\.test: some TAP diagnostic, will go to console$' stdout
+grep '^# some TAP diagnostic, will go to console$' test-suite.log
 grep '^some non-TAP text, will be copied in the global log$' test-suite.log
-grep '^PASS: baz\.test 1 - seen also if the whole test is skipped!$' stdout
-test `grep -c ': .*\.test' stdout` -eq 4
-count_test_results total=4 pass=1 fail=0 xpass=0 xfail=0 skip=3 error=0
+test `grep -c ': .*\.test' stdout` -eq 2
+count_test_results total=2 pass=0 fail=0 xpass=0 xfail=0 skip=2 error=0
 
 :
diff --git a/tests/tap-skip-whole-unplanned.test 
b/tests/tap-skip-whole-unplanned.test
new file mode 100755
index 0000000..6286dac
--- /dev/null
+++ b/tests/tap-skip-whole-unplanned.test
@@ -0,0 +1,73 @@
+#! /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:
+#  - a "plan with skip" given after one or more test result have already
+#    been seen is an error
+#  - any test result following a "plan with skip" is an error.
+
+parallel_tests=yes
+. ./defs || Exit 1
+
+. "$testsrcdir"/tap-setup.sh || fatal_ "sourcing tap-setup.sh"
+
+cat > all.test <<END
+ok 1
+1..0 # SKIP too late
+END
+$MAKE check >stdout && { cat stdout; Exit 1; }
+cat stdout
+count_test_results total=2 pass=1 fail=0 xpass=0 xfail=0 skip=0 error=1
+grep '^ERROR: all\.test - too many tests run (expected 0, got 1)$' stdout
+
+cat > all.test <<END
+ok 1
+ok 2 # SKIP
+not ok 3 # TODO
+1..0 # SKIP too late
+END
+$MAKE check >stdout && { cat stdout; Exit 1; }
+cat stdout
+count_test_results total=4 pass=1 fail=0 xpass=0 xfail=1 skip=1 error=1
+grep '^ERROR: all\.test - too many tests run (expected 0, got 3)$' stdout
+
+cat > all.test <<END
+1..0 # SKIP falsified later
+ok 1
+END
+$MAKE check >stdout && { cat stdout; Exit 1; }
+cat stdout
+count_test_results total=3 pass=0 fail=0 xpass=0 xfail=0 skip=1 error=2
+grep '^ERROR: all\.test 1 # UNPLANNED$' stdout
+grep '^ERROR: all\.test - too many tests run (expected 0, got 1)$' stdout
+
+cat > all.test <<END
+1..0 # SKIP falsified later
+ok 1
+ok 2 # SKIP
+not ok 3
+not ok 4 # TODO
+END
+$MAKE check >stdout && { cat stdout; Exit 1; }
+cat stdout
+count_test_results total=6 pass=0 fail=0 xpass=0 xfail=0 skip=1 error=5
+grep '^ERROR: all\.test 1 # UNPLANNED$' stdout
+grep '^ERROR: all\.test 2 # UNPLANNED$' stdout
+grep '^ERROR: all\.test 3 # UNPLANNED$' stdout
+grep '^ERROR: all\.test 4 # UNPLANNED$' stdout
+grep '^ERROR: all\.test - too many tests run (expected 0, got 4)$' stdout
+
+:
diff --git a/tests/tap-skip-whole.test b/tests/tap-skip-whole.test
index 6b14be4..2852305 100755
--- a/tests/tap-skip-whole.test
+++ b/tests/tap-skip-whole.test
@@ -26,44 +26,35 @@ weirdchars=\''"$!&()[]<>#;^?*/@%=,.:'
 
 cat > foo.test <<END
 1..0
-not ok 1
-not ok 2
 END
 
 cat > bar.test <<END
+blah
+# blah
 1..0$tab $tab
-ok 1
-Bail out! some random failure
 END
 
 # It is undefined whether the comment after the plan below should
 # count as an explanation; still, the test should be skipped.
 cat > baz.test <<END
 1..0 # WWW::Mechanize not installed
-ok 1
+other
+    junk
+       lines
 END
 
 cat > wget.test <<END
 1..0 # SKIP wget(1) not installed
-not ok 1 # TODO
-ok 2
-ok 3 # SKIP
+# See also curl.test
 END
 
 cat > curl.test <<END
 1..0 # skip: Can't connect to gnu.org!
-not ok 1
-ok 2 # TODO
+# See also wget.test
 END
 
 cat > mu.test <<END
 1..0 # Skip $weirdchars
-# Various TAP errors in here shouldn't be seen, as the test is skipped.
-1..2
-ok 1
-ok 22
-not ok 333
-1..7
 END
 
 env TESTS='foo.test bar.test baz.test wget.test curl.test mu.test' \
-- 
1.7.2.3