Merge lp:~gl-az/percona-xtrabackup/2.0-testrun-summary into lp:percona-xtrabackup/2.0

[George Ormond Lorch III]

Added test status tracking and summary report at end of test run cycle to make it wasy to see what failed at a glance, similar to MTR.
</facepalm>

[Laurynas Biveinis (laurynas-biveinis)]

Doesn't this report the skipped testcases (exit code == 200) as the failed ones?

If yes, that wouldn't be entirely correct, I'd report failed and skipped testcases separately.

Typo on diff line 32.

review: Needs Fixing

[George Ormond Lorch III (gl-az)]

Good catch Laurynas, I didn't think about the skipped cases in this context. Something else worth considering is making the testrun behave more like the suite and MTR in that it will stop on the first failure and return that error code, which can be overridden by a -f and maybe add some retry logic.

[Laurynas Biveinis (laurynas-biveinis)]

It looks that now the skipped cases will be listed twice: both as skipped (exit == 200), and as failed (exit != 0).

review: Needs Fixing

[George Ormond Lorch III (gl-az)]

Oy, Doh! That's what I get for trying to do n+1 things at once where 'n'
is my max capacity for parallelism...

On 6/3/2013 9:34 PM, Laurynas Biveinis wrote:
> Review: Needs Fixing
>
> It looks that now the skipped cases will be listed twice: both as skipped (exit == 200), and as failed (exit != 0).
>

--
George O. Lorch III
Software Engineer, Percona
[phone] x542 US/Arizona (GMT -7)
skype: george.ormond.lorch.iii

[George Ormond Lorch III (gl-az)]

OK Laurynas, as we discussed on IRC I did some more testing:
Created a new 'suite' called 'u' which exists as a sibling directory to the current 't' (test/u).
Added 3 simple tests:

$ cat u/test_pass.sh
. inc/common.sh
sleep 5
exit 0

$ cat u/test_fail.sh
. inc/common.sh
sleep 5
exit 1

$ cat u/test_skip.sh
. inc/common.sh
sleep 5
exit $SKIPPED_EXIT_CODE

Then ran the suite:
$ ./testrun -j 1 -s u

The first thing I ran into was all tests were failing with 255, why? Well, testrun.c doesn't work with alternate suites correctly. It will find the tests in the correct location, but when it goes to execute them, it tries to execute them out of ./t/<testname> instead of ./<suitename>/<testname>. This was a simple fix so I made it and updated the branch with this fix.

With that working, I run again and realize that testrun.c launches all tests individually through testrun.sh. testrun.sh only returns success (0) or failure (-1/255), nothing else because it may have run multiple tests so it only returns 'all succeeded or skipped' or 'something failed'. With this, in order to report skipped tests, we will need to modify testrun.sh to do something like pass the result from the tests directly out of there was only one test specified, otherwise, use the old method.

So, that being said, we have two choices here:
1) We just go back to my original work where we were really only interested in reporting true failures and accepting skipped tests as passed.
2) Change testrun.sh to return the true result when only one test was specified.

Opinions?

[lp:~gl-az/percona-xtrabackup/2.0-testrun-summary]

561. By George Ormond Lorch III on 2013-06-05

Fixed issue in testrun.c where specifying testsuite would find the tests in the corredt directory but would still try to execute them out of ./t

Added test status tracking and summary report at end of test run cycle to make it wasy to see what passed, skipped and failed at a glance, similar to MTR.

Changed testrun.sh slightly to return same code out of testrun.sh as the test that was executed when only a single test was run.

[George Ormond Lorch III (gl-az)]

So I went ahead and took a peek into testrun.sh to see about returning the true test return status when only one test was being run and it turned out to be only a few, somewhat ugly, lines of code but it works now. Also found and fixed the logic error that I think Laurynas was getting at during IRC conversation. This branch now seems to work and detect all cases correctly:

$ ./testrun -j 1 -s u
./testrun running with: -s u -j 1 -t 600

Found 3 testcases
[0] LAUNCHING - test_fail.sh
[0] completed test_fail.sh status 1
[0] LAUNCHING - test_pass.sh

nrchildren= 1, 1 tests remaining
Running: test_pass.sh
[0] completed test_pass.sh status 0
[0] LAUNCHING - test_skip.sh

nrchildren= 1, 0 tests remaining
Running: test_skip.sh
[0] completed test_skip.sh status 200

nrchildren= 0, 0 tests remaining
Running:
SUMMARY: 3 run, 1 successful, 1 skipped, 1 failed
Skipped cases: test_skip.sh
Failing cases: test_fail.sh

If this all looks acceptable then I will go ahead and merge forward to 2.1 as well and resubmit MP there.

[Stewart Smith (stewart)]

George Ormond Lorch III <email address hidden> writes:
> SUMMARY: 3 run, 1 successful, 1 skipped, 1 failed
> Skipped cases: test_skip.sh
> Failing cases: test_fail.sh

I like the summary. While we could no doubt do the same thing by parsing
the subunit output, I don't think that's needed (and is much more
complex, or involves having a dependency on some random python utility).

--
Stewart Smith

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

Unmerged revisions

561. By George Ormond Lorch III on 2013-06-05

Fixed issue in testrun.c where specifying testsuite would find the tests in the corredt directory but would still try to execute them out of ./t

Added test status tracking and summary report at end of test run cycle to make it wasy to see what passed, skipped and failed at a glance, similar to MTR.

Changed testrun.sh slightly to return same code out of testrun.sh as the test that was executed when only a single test was run.

=== modified file 'test/testrun.c'
--- test/testrun.c	2012-11-30 04:28:06 +0000
+++ test/testrun.c	2013-06-05 21:04:26 +0000
@@ -41,6 +41,7 @@
   char *buf;
   size_t end_of_buf;
   size_t bufsz;
+  int status;
 };
 
 pid_t *childpid;
@@ -96,6 +97,7 @@
     (*cases)[i].buf= NULL;
     (*cases)[i].end_of_buf= 0;
     (*cases)[i].bufsz= 0;
+    (*cases)[i].status= 0;
     free(namelist[i]);
   }
 
@@ -113,7 +115,41 @@
   free(cases);
 }
 
-static int run_testcase_in_child(int nr, struct testcase *t, pid_t *cpid, const char* xbtarget)
+static void report_status(struct testcase *cases, int n)
+{
+  int i;
+  int failed=0;
+  int skipped=0;
+  for (i=0; i<n; i++)
+    if (WEXITSTATUS(cases[i].status) != 0)
+      if (WEXITSTATUS(cases[i].status) == 200)
+        skipped++;
+      else
+        failed++;
+
+  printf("SUMMARY: %d run, %d successful, %d skipped, %d failed\n", n, n - failed - skipped, skipped, failed);
+  if (skipped)
+  {
+    printf("Skipped cases:");
+    for (i=0; i<n; i++) 
+      if (WEXITSTATUS(cases[i].status) == 200)
+        printf(" %s", cases[i].name);
+    printf("\n");
+  }
+
+  if (failed)
+  {
+    printf("Failing cases:");
+    for (i=0; i<n; i++) 
+      if (WEXITSTATUS(cases[i].status) != 0 && WEXITSTATUS(cases[i].status) != 200)
+        printf(" %s", cases[i].name);
+    printf("\n");
+  }
+}
+
+static int run_testcase_in_child(const char* suitedir, int nr,
+				 struct testcase *t, pid_t *cpid,
+				 const char* xbtarget)
 {
   int fd[2];
 
@@ -132,7 +168,7 @@
     close(fd[0]);
 
     char tname[500];
-    snprintf(tname, sizeof(tname), "t/%s",t->name);
+    snprintf(tname, sizeof(tname), "%s/%s",suitedir, t->name);
 
     char basedir[PATH_MAX];
     char cwd[PATH_MAX];
@@ -195,14 +231,14 @@
   free(buf);
 }
 
-static void run_testcases(struct testcase *testcases, int nrcases,
-			  int njobs, int timeout, const char* xbtarget)
+static void run_testcases(const char* suitedir, struct testcase *testcases,
+			  int nrcases, int njobs, int timeout,
+			  const char* xbtarget)
 {
   int childfd[njobs];
   int nfds= 0;
   int retval;
   pid_t chpid[njobs];
-  int status;
   int next_testcase= 0;
   int i;
   fd_set rfds;
@@ -223,7 +259,7 @@
   for(i=0; i<njobs; i++)
   {
     childtest[i]=next_testcase++;
-    childfd[i]= run_testcase_in_child(i, &testcases[childtest[i]], &childpid[i], xbtarget);
+    childfd[i]= run_testcase_in_child(suitedir, i, &testcases[childtest[i]], &childpid[i], xbtarget);
   }
 
   fflush(stdout);
@@ -277,8 +313,8 @@
 	    t->end_of_buf+= r;
 	} while(r>0);
 
-	pid_t waited= waitpid(childpid[i], &status, WNOHANG);
-	if (!(WIFEXITED(status) || WIFSIGNALED(status)))
+	pid_t waited= waitpid(childpid[i], &testcases[childtest[i]].status, WNOHANG);
+	if (!(WIFEXITED(testcases[childtest[i]].status) || WIFSIGNALED(testcases[childtest[i]].status)))
 	  continue;
 
 	if (waited != childpid[i])
@@ -289,14 +325,14 @@
 
 	close(childfd[i]);
 	printf("[%d] completed %s status %d\n",
-	       i, testcases[childtest[i]].name, WEXITSTATUS(status));
+	       i, testcases[childtest[i]].name, WEXITSTATUS(testcases[childtest[i]].status));
 	childfd[i]=-1;
 	nchildren--;
 
 	if (next_testcase < nrcases)
 	{
 	  childtest[i]=next_testcase++;
-	  childfd[i]= run_testcase_in_child(i, &testcases[childtest[i]], &childpid[i], xbtarget);
+	  childfd[i]= run_testcase_in_child(suitedir, i, &testcases[childtest[i]], &childpid[i], xbtarget);
 	  nfds= (childfd[i] > nfds)? childfd[i] : nfds;
 	  nchildren++;
 	}
@@ -397,7 +433,9 @@
 
   printf("Found %d testcases\n", nrcases);
 
-  run_testcases(testcases, nrcases, njobs, timeout, xbtarget);
+  run_testcases(suitedir, testcases, nrcases, njobs, timeout, xbtarget);
+
+  report_status(testcases, nrcases);
 
   free_testcases(testcases, nrcases);
 
 
=== modified file 'test/testrun.sh'
--- test/testrun.sh	2013-05-07 07:53:25 +0000
+++ test/testrun.sh	2013-06-05 21:04:26 +0000
@@ -284,6 +284,7 @@
 
 failed_count=0
 failed_tests=
+skipped_count=0
 total_count=0
 
 export OUTFILE="$PWD/results/setup"
@@ -337,6 +338,7 @@
        test -r $SKIPPED_REASON && sreason=`cat $SKIPPED_REASON`
        echo "[skipped]    $sreason"
        subunit_skip_test $t >> $SUBUNIT_OUT
+       skipped_count=`expr $skipped_count + 1`
    else
        echo "[failed]"
 
@@ -348,7 +350,7 @@
 
        failed_count=$((failed_count+1))
        failed_tests="$failed_tests $t"
-       result=1
+       result=$rc
        if [ -z "$force" ]
        then
 	   break;
@@ -359,10 +361,18 @@
 echo "========================================================================"
 echo
 
-if [ $result -eq 1 ]
+if [ $result -ne 0 ]
 then
     echo
     echo "$failed_count/$total_count tests have failed: $failed_tests"
     echo "See results/ for detailed output"
-    exit -1
+    if [ $total_count -gt 1 ]
+    then
+      exit -1
+    else
+      exit $result
+    fi
+elif [ $total_count -eq 1 ] && [ $skipped_count -eq 1 ]
+then
+    exit $SKIPPED_EXIT_CODE
 fi