Allow configurable "Advisory" reviews

I have another use case that requires advisory test results. It's the integration between Text Review and Coder. It was supposed to be deployed on the testing servers, but since all it does is review stuff (and not test it), it should not be able to stop a patch from being accepted. It should instead result in a message saying "Hi there, your UI texts look a bit sloppy to me.".

Bottom line: we need a way to return advisory test results that don't block patch acceptance, but still allow QA to tell the issue queue something might be wrong.

Here's a first crack at an untested patch ... an environment would be set as 'advisory' by setting a non-empty 'advisory' plugin_argument on the environment.

It's a start ... but the patch appears to interfere with the passing of the result code back to PIFR_Server.

Fresh start ...

First patch adds 'pifr_server_environments_is_advisory()', and includes an 'overall result' row in the environment status test table for tests with multiple environments (with failed advisory environments not contributing to a overall 'fail' status).

Next step ...

Added advisory review considerations to the test page 'summary message'.

Step three ...

Take advisory environments into account when checking/communicating the actual overall test status.

Step four ...

Only ignore advisory results if there are multiple results in the set. If there is only one result, then treat it as before. (This ensures that client tests still receive their 'failure' results when expected.)

Almost worked ... ended up failing on client confirmation tests when the client runs multiple environments.

This patch should address that issue.

Next Issue: 'Overall Result' should only show up when there are results ... two 'queued' environments does not equal a pass!

Fix included in this patch.

And BOOM goes the dynamite!

http://rfay.redesign.devdrupal.org/node/1119121#comment-4315798

You'll need at least one testbot set up to handle an environment, if that environment is active on qa.scratch. In this case, when my local testbot went away, there was nothing left to perform the coder reviews. If scratchtestbot was set up to handle them, I think things would have been fine.

A quick way to disable the coder environment is to go to administer -> pifr -> environment -> edit -> conditions, and add branch 133700000 to the Coder environment. This is how it was initially disabled - deleting it isn't really necessary.

As for the environment deletion being broken ... I suspect that's always been there.

I don't think the delayed test results when the coder testbot went away is cause for concern ... PIFT was designed to not send back a response until it has heard back from all test environments.

For production deployment, I would suggest that Coder would be turned on for multiple testbots so that failure of one would not delay test results ... the tests would simply be picked up by the next guy. This shouldn't cause a significant performance issue either, as the coder runs only take a couple of minutes.

And for your example, the test is 'queued' until sent to all environments ... until you enabled a testbot accepting coder tests, queued is the correct status (since not all tests have been 'sent' yet).

The only concern I've noticed is that the coder client verification tests seem to be randomly failing more often than the simpletest tests ... which probably deserves a seperate issue.

Given the above, moving back to 'needs review'.

Notes from an IRC review (Thanks, chx!!!)

+++ b/server/pifr_server.environment.inc
@@ -593,3 +593,21 @@ function pifr_server_environment_save(array $environment) {
+function pifr_server_environment_is_advisory($environment) {
+  if (!empty($environment['plugin_argument']['advisory'])) {
+    return TRUE;
+  }
+  return FALSE;
+}

return !empty($environment['plugin_argument']['advisory']);

+++ b/server/pifr_server.result.inc
@@ -84,8 +84,18 @@ function pifr_server_result_get_all($test_id) {
+    if ((count($results) < 2)

if ((count($results) == 1)

+++ b/server/pifr_server.test.inc
@@ -905,16 +905,34 @@ abstract class PIFRTest {
+    $advisorypass = TRUE;

$advisorypass = 'pass';

+++ b/server/pifr_server.test.inc
@@ -905,16 +905,34 @@ abstract class PIFRTest {
+            $advisorypass = FALSE;

$advisorypass = 'fail';

+++ b/server/pifr_server.test.inc
@@ -905,16 +905,34 @@ abstract class PIFRTest {
+      $rows[] = array(t("Overall Result"), ($advisorypass ? 'pass' : 'fail'));

$rows[] = array(t("Overall Result"), $advisorypass);

Code cleanup - Updated patch with changes from #16.

Committed to 6.x-2.x (0ae039a)