Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ayang’s picture

Status: Needs review » Needs work

The last submitted patch, 2088603-acquia_connector-user_contrib_tests.patch, failed testing.

ayang’s picture

Status: Needs work » Needs review
FileSize
16.84 KB

Git patch.

ayang’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev
FileSize
18.38 KB

Drupal 6 backport.

coltrane’s picture

Status: Needs review » Needs work

Users are a bit confused about the Acquia Connector being multiple modules and there's a JIRA issue to eventually combine Agent and SPI into one module. For that reason I don't think custom tests should be a separate module. Would you be OK with moving this all to Acquia SPI? acquia_spi_test_collect() could become part of acquia_spi.module just fine.

I'm not convinced of the usefulness of storing test failures and providing a page callback. I understand developers would like to validate their tests but I would like to see if there's a lighter weight solution. Perhaps a "validate custom tests" link on the admin reports status page that either watchdog()s or prints the results in a drupal_set_message()? Or perhaps a drush command for test validation? I think authors of custom tests would be comfortable with drush. What do you think?

ayang’s picture

Per our discussion earlier, that shouldn't be a problem. I'll move the relevant functions over into the Acquia SPI module. I can go either way on the best way to log these, but I don't mind going for the lighter weight option. I'll also add a drush command the next iteration.

ayang’s picture

Added the drush command and merged everything into Acquia SPI.

Status: Needs review » Needs work

The last submitted patch, 2088603-acquia_connector-user_contrib_tests_d7-7.patch, failed testing.

ayang’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2088603-acquia_connector-user_contrib_tests_d7-7.patch, failed testing.

ayang’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev
Status: Needs work » Needs review
FileSize
12.47 KB

Switching review to D7.

coltrane’s picture

Looking good and functional! Just some minor updates I think.

  1. +++ b/acquia_spi/acquia_spi.drush.inc
    @@ -19,6 +20,11 @@ function acquia_spi_drush_command() {
    +  $items['spi-validate'] = array(
    

    How about spi-test-validate?

  2. +++ b/acquia_spi/acquia_spi.install
    @@ -75,6 +75,32 @@ function acquia_spi_requirements($phase) {
    +          $description = 'Custom tests within the following module(s) have failed validation and will not be sent: %modules. <br/>Please check your error logs for more information regarding how to pass validation.';
    

    Could also recommend the drush command here. You could add a help link here and some text like the following to acquia_agent.module:

    diff --git a/acquia_agent/acquia_agent.module b/acquia_agent/acquia_agent.module
    index eb865af..f9b4d0a 100644
    --- a/acquia_agent/acquia_agent.module
    +++ b/acquia_agent/acquia_agent.module
    @@ -551,6 +551,8 @@ function acquia_agent_help($path, $arg) {
           $output .= '<dd>' . t('Receive dynamic updates on the Network Settings page from Acquia.com about your subscription and new features.') . '</dd>';
           $output .= '<dt>' . t('Allow Insight to update list of approved variables.') . '</dt>';
           $output .= '<dd>' . t('As part of the Acquia Insight service, some variables can be corrected to their recommended settings from within the Insight system. The list of variables that can be corrected can also be updated at your discretion.') . '</dd>';
    +      $output .= '<dt>' . t('Acquia SPI custom tests.') . '</dt>';
    +      $output .= '<dd>' . t('Acquia Insight supports custom tests for your site. See acquia_spi.api.php for information on the custom test hook and validate your tests for inclusion in outgoing SPI data with the Drush command "spi-test-validate".') . '</dd>';
           $output .= '</dl>';
           return $output;
       }
    
  3. +++ b/acquia_spi/acquia_spi.module
    @@ -598,6 +605,164 @@ function _acquia_spi_security_review_compatible() {
    + * Validates data from callbacks.
    

    "from custom test callbacks"

  4. +++ b/acquia_spi/acquia_spi.api.php
    @@ -33,3 +33,33 @@ function hook_acquia_spi_get() {
    + * careful not to send sensitive information in any field.
    

    This hook supports multiple tests so long as they each have a unique identifier right? Perhaps add a comment saying so.

  5. +++ b/acquia_spi/acquia_spi.module
    @@ -598,6 +605,164 @@ function _acquia_spi_security_review_compatible() {
    +
    

    For string attributes, how about also limiting the length? That way someone can't send more than X characters in a field (unless they hack it, but then we don't have to support it). Maybe a limit of 1024 characters? Please also add to the hook documentation.

ayang’s picture

Added these updates:

  1. Makes sense to make it a bit more specific. Changed it.
  2. Added the help link and a menu callback to perform a validation check. Added a help section to Acquia Agent, and also added "more information" links onto the status page.
  3. Whoops! Added.
  4. Cleaned up the acquia_spi.api.php a bit and added notes about the unique identifiers.
  5. Added the string character cap and notes for it.
coltrane’s picture

I wasn't clear in chat about the access arguments, sorry, thought there was a permission on that callback. I modified the test validate callback to use permission 'access site reports'. Also added note about character count to error message.

This looks good, thanks so much for your work on this @ayang!!

I'll be committing these two versions after testbot shows d7 passing.

coltrane’s picture

ayang’s picture

:D

Status: Fixed » Closed (fixed)

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