[PATCH] tap: improve diagnosing and reporting of plan mismatches

[Stefano Lattarini]

Problem exposed by a failure in the test 'tap-plan-errors.test'.

* lib/tap-driver (NO_PLAN, EARLY_PLAN, LATE_PLAN): New constants.
(Throughout the file): Use more complex semantics for `$plan_seen',
which now also remember whether the plan that has been seen is an
"early" or "late" plan; in turn, this makes ...
($tap_stopped): ... this variable redundant; remove it.
(handle_tap_plan): Adjust to avoid producing spurious or confusing
error messages about extra or mismatched "late" TAP plans.
* tests/tap-plan-corned.test: Adjust and extend.
* tests/tap-color.test: Likewise.
---
 ChangeLog                  |   14 ++++++++
 lib/tap-driver             |   39 +++++++++++++++--------
 tests/tap-color.test       |   10 +++++-
 tests/tap-plan-corner.test |   72 ++++++++++++++++++++++++++++++++++++--------
 4 files changed, 106 insertions(+), 29 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index f3b0684..93764b7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,19 @@
 2011-08-12  Stefano Lattarini  <address@hidden>
 
+       tap: improve diagnosing and reporting of plan mismatches
+       Problem exposed by a failure in the test 'tap-plan-errors.test'.
+       * lib/tap-driver (NO_PLAN, EARLY_PLAN, LATE_PLAN): New constants.
+       (Throughout the file): Use more complex semantics for `$plan_seen',
+       which now also remember whether the plan that has been seen is an
+       "early" or "late" plan; in turn, this makes ...
+       ($tap_stopped): ... this variable redundant; remove it.
+       (handle_tap_plan): Adjust to avoid producing spurious or confusing
+       error messages about extra or mismatched "late" TAP plans.
+       * tests/tap-plan-corned.test: Adjust and extend.
+       * tests/tap-color.test: Likewise.
+
+2011-08-12  Stefano Lattarini  <address@hidden>
+
        tap: log all TAP stream, even after a "Bail out!"
        * lib/tap-driver ($bailed_out): New global boolean variable,
        telling whether a "Bail out!" directive has been seen or not.
diff --git a/lib/tap-driver b/lib/tap-driver
index a1dacdb..95fb78c 100755
--- a/lib/tap-driver
+++ b/lib/tap-driver
@@ -39,18 +39,22 @@ my %COLOR = (
   std => "\e[m",
 );
 
+# It's important that NO_PLAN evaluates "false" as a boolean.
+use constant NO_PLAN => 0;
+use constant EARLY_PLAN => 1;
+use constant LATE_PLAN => 2;
+
 # ------------------- #
 #  Global variables.  #
 # ------------------- #
 
 my $testno = 0;     # Number of test results seen so far.
-my $plan_seen = 0;  # Whether the TAP plan has been seen or not.
 my $bailed_out = 0; # Whether a "Bail out!" directive has been seen.
 my $parser;         # TAP parser object (will be initialized later).
 
-# When true, it means that the rest of the input stream cannot
-# contain any further TAP results.
-my $tap_stopped = 0;
+# Whether the TAP plan has been seen or not, and if yes, which kind
+# it is ("early" is seen before any test result, "late" otherwise).
+my $plan_seen = NO_PLAN;
 
 # ----------------- #
 #  Option parsing.  #
@@ -238,7 +242,9 @@ sub stringify_test_result ($)
   my $result = shift;
   my $PASS = $cfg{"expect-failure"} ? "XPASS": "PASS";
   my $FAIL = $cfg{"expect-failure"} ? "XFAIL": "FAIL";
-  if ($result->is_unplanned || $result->number != $testno || $tap_stopped)
+  if ($result->is_unplanned
+       || $result->number != $testno
+       || $plan_seen == LATE_PLAN)
     {
       return "ERROR";
     }
@@ -329,7 +335,7 @@ sub handle_tap_test ($)
       $string .= " $description";
     }
 
-  if ($tap_stopped)
+  if ($plan_seen == LATE_PLAN)
     {
       $string .= " # AFTER LATE PLAN";
     }
@@ -356,25 +362,30 @@ sub handle_tap_test ($)
 sub handle_tap_plan ($)
 {
   my $plan = shift;
-  # Only one plan per stream is acceptable.
-  testsuite_error "multiple test plans" if $plan_seen;
-  # 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;
+  if ($plan_seen)
+    {
+      # Error, only one plan per stream is acceptable.
+      testsuite_error "multiple test plans";
+      return;
+    }
+  # The TAP plan can come before or after *all* the TAP results; we speak
+  # respectively of an "early" or a "late" plan.  If we see the plan line
+  # after at least one TAP result has been seen, assume we have a late
+  # plan; in this case, any further test result seen after the plan will
+  # be flagged as an error.
+  $plan_seen = ($testno >= 1 ? LATE_PLAN : EARLY_PLAN);
   # If $testno > 0, we have an error ("too many tests run") 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.  Otherwise, we have a valid "plan
   # with SKIP" specification, and should report it as a particular kind
   # of SKIP result.
-  if ($plan->directive && $testno == 0 && !$plan_seen)
+  if ($plan->directive && $testno == 0)
     {
       my $explanation = $plan->explanation ?
                         "- " . $plan->explanation : undef;
       report "SKIP", $explanation;
     }
-  $plan_seen = 1;
 }
 
 sub handle_tap_bailout ($)
diff --git a/tests/tap-color.test b/tests/tap-color.test
index cd0329b..16fea15 100755
--- a/tests/tap-color.test
+++ b/tests/tap-color.test
@@ -44,7 +44,7 @@ AUTOMAKE_OPTIONS = color-tests
 AM_TEST_LOG_DRIVER_FLAGS = --comments
 TEST_LOG_COMPILER = cat
 TESTS = all.test skip.test bail.test badplan.test noplan.test \
-        few.test many.test order.test
+        few.test many.test order.test afterlate.test
 END
 
 . "$testsrcdir"/tap-setup.sh || fatal_ "sourcing tap-setup.sh"
@@ -96,6 +96,12 @@ cat > order.test << 'END'
 ok 5
 END
 
+cat > afterlate.test << 'END'
+ok 1
+1..2
+ok 2
+END
+
 AM_COLOR_TESTS=always $MAKE check >stdout && { cat stdout; Exit 1; }
 cat stdout
 
@@ -111,12 +117,12 @@ test_color ()
   cat stdout | grep "^${grn}PASS${std}: bail\.test 1$"
   cat stdout | grep "^${mgn}ERROR${std}: bail\.test - Bail out!$"
   cat stdout | grep "^${mgn}ERROR${std}: badplan\.test - multiple test plans$"
-  cat stdout | grep "^${mgn}ERROR${std}: badplan\.test 2 # AFTER LATE PLAN$"
   cat stdout | grep "^${mgn}ERROR${std}: noplan\.test - missing test plan$"
   cat stdout | grep "^${mgn}ERROR${std}: few.test - too few tests run 
(expected 2, got 1)$"
   cat stdout | grep "^${mgn}ERROR${std}: many.test - too many tests run 
(expected 1, got 2)$"
   cat stdout | grep "^${mgn}ERROR${std}: many.test 2 # UNPLANNED$"
   cat stdout | grep "^${mgn}ERROR${std}: order.test 5 # OUT-OF-ORDER 
(expecting 1)$"
+  cat stdout | grep "^${mgn}ERROR${std}: afterlate\.test 2 # AFTER LATE PLAN$"
   # Diagnostic messages shouldn't be colorized.
   cat stdout | grep "^# all\.test: Hi! I shouldn't be colorized!$"
   :
diff --git a/tests/tap-plan-corner.test b/tests/tap-plan-corner.test
index a7c9933..a818344 100755
--- a/tests/tap-plan-corner.test
+++ b/tests/tap-plan-corner.test
@@ -22,6 +22,8 @@ parallel_tests=yes
 
 . "$testsrcdir"/tap-setup.sh || fatal_ "sourcing tap-setup.sh"
 
+# -------------------------------------------------------------------------
+
 cat > leading-repeated.test <<END
 1..1
 1..1
@@ -42,6 +44,8 @@ for pos in leading trailing; do
   grep "^ERROR: $pos-repeated\\.test - multiple test plans$" stdout
 done
 
+# -------------------------------------------------------------------------
+
 cat > leading-repeated.test <<END
 1..2
 ok 1
@@ -59,11 +63,13 @@ END
 env TESTS="leading-repeated.test trailing-repeated.test" \
   $MAKE -e check >stdout && { cat stdout; Exit 1; }
 cat stdout
-count_test_results total=6 pass=2 fail=0 xpass=0 xfail=0 skip=0 error=4
-for pos in leading trailing; do
-  grep "^ERROR: $pos-repeated\\.test - multiple test plans$" stdout
-  grep "^ERROR: $pos-repeated\\.test 2 # AFTER LATE PLAN$" stdout
-done
+count_test_results total=6 pass=3 fail=0 xpass=0 xfail=0 skip=0 error=3
+grep "^ERROR: leading-repeated\\.test - multiple test plans$" stdout
+grep "^ERROR: trailing-repeated\\.test - multiple test plans$" stdout
+grep "^ERROR: trailing-repeated\\.test 2 # AFTER LATE PLAN$" stdout
+grep "leading .*AFTER LATE PLAN" stdout && Exit 1
+
+# -------------------------------------------------------------------------
 
 cat > 1.test <<END
 1..0
@@ -93,6 +99,8 @@ 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
@@ -107,17 +115,17 @@ END
 
 $MAKE -e check >stdout && { cat stdout; Exit 1; }
 cat stdout
-count_test_results total=8 pass=2 fail=0 xpass=0 xfail=0 skip=0 error=6
+count_test_results total=8 pass=5 fail=0 xpass=0 xfail=0 skip=0 error=3
 
 cat > exp <<'END'
 PASS: all.test 1
 PASS: all.test 2
 ERROR: all.test - multiple test plans
-ERROR: all.test 3 # AFTER LATE PLAN
+PASS: all.test 3
 ERROR: all.test - multiple test plans
-ERROR: all.test 4 # AFTER LATE PLAN
+PASS: all.test 4
 ERROR: all.test - multiple test plans
-ERROR: all.test 5 # AFTER LATE PLAN
+PASS: all.test 5
 END
 
 $FGREP ': all.test' stdout > got
@@ -126,17 +134,55 @@ cat exp
 cat got
 diff exp got
 
-sed -e 1d all.test > t
-mv -f t all.test
+# -------------------------------------------------------------------------
+
+cat > all.test <<END
+1..2
+ok 1
+ok 2
+1..3
+ok 3
+END
+
+$MAKE -e check >stdout && { cat stdout; Exit 1; }
+cat stdout
+count_test_results total=5 pass=2 fail=0 xpass=0 xfail=0 skip=0 error=3
+
+cat > exp <<'END'
+PASS: all.test 1
+PASS: all.test 2
+ERROR: all.test - multiple test plans
+ERROR: all.test 3 # UNPLANNED
+ERROR: all.test - too many tests run (expected 2, got 3)
+END
+
+$FGREP ': all.test' stdout > got
+
+cat exp
+cat got
+diff exp got
+
+# -------------------------------------------------------------------------
+
+cat > all.test <<END
+ok 1
+ok 2
+ok 3
+1..5
+1..5
+ok 4
+1..5
+ok 5
+END
 
 $MAKE -e check >stdout && { cat stdout; Exit 1; }
 cat stdout
-count_test_results total=7 pass=2 fail=0 xpass=0 xfail=0 skip=0 error=5
+count_test_results total=7 pass=3 fail=0 xpass=0 xfail=0 skip=0 error=4
 
 cat > exp <<'END'
 PASS: all.test 1
 PASS: all.test 2
-ERROR: all.test 3 # AFTER LATE PLAN
+PASS: all.test 3
 ERROR: all.test - multiple test plans
 ERROR: all.test 4 # AFTER LATE PLAN
 ERROR: all.test - multiple test plans
-- 
1.7.2.3