I think we're going to have a hard time actually deploying coder, for example, because there will be lots of #fails at first. If those #fails call HEAD broken, for example, it's just too much work to clean up. But if a new environment could be "advisory" for a few months, maybe we'd have a chance. This would work for PostgreSQL as well.

Comments

Xano’s picture

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.

jthorson’s picture

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.

jthorson’s picture

Status: Active » Needs work

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

jthorson’s picture

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).

jthorson’s picture

Next step ...

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

jthorson’s picture

Step three ...

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

jthorson’s picture

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.)

jthorson’s picture

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

This patch should address that issue.

jthorson’s picture

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.

jthorson’s picture

Status: Needs work » Needs review
rfay’s picture

That is absolutely STUPENDOUS! (I didn't follow the link before :-)
Took me a while to find the coder tab, but finally found it.

People are going to be so happy with you!

Did you not even have to make any changes to pift? Doesn't look like it.

rfay’s picture

Status: Needs review » Needs work

With this deployed on qa.scratch I did have some trouble.

I was testing a junk patch on qa.scratch.

Coder was enabled on qa.scratch, but not on scratchtestbot.

I couldn't ever get the patch to complete. The testbot tested it successfully and qa got the result (but still showed coder as queued). So PIFT never got the response.

It seems that perhaps "queued" prevents the correct advisory behavior.

I'm going to clean up PIFR on qa.scratch and see if I can get out of this.

rfay’s picture

Deleting an environment (as I just did on qa.scratch) and then trying to retest gets a fatal:

Recoverable fatal error: Argument 2 passed to pifr_server_test_xmlrpc() must be an array, boolean given, called in /var/www/qa.scratch.drupal.org/htdocs/sites/all/modules/project_issue_file_review/server/pifr_server.test.inc on line 726 and defined in pifr_server_test_xmlrpc() (line 213 of /var/www/qa.scratch.drupal.org/htdocs/sites/all/modules/project_issue_file_review/server/pifr_server.test.inc).

It looks like the proper rows don't get deleted from the pifr_test_environment table when an environment is deleted.

In this case the environment id was 2; I cleaned up with

delete from pifr_test_environment where environment_id = 2;
jthorson’s picture

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.

jthorson’s picture

Status: Needs work » Needs review

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'.

jthorson’s picture

Status: Needs review » Needs work

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);

jthorson’s picture

Status: Needs work » Needs review
FileSize
4.33 KB
None View

Code cleanup - Updated patch with changes from #16.

jthorson’s picture

Status: Needs review » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.