Change getInfo() to a static method

Initial hook design. I'm think about removing file_path and just assuming that if you set file key then the file path should be /path/to/module/tests. Thoughts?

Current hook API:

/**
 * Define tests provided.
 *
 * This hook allows modules to add tests to global list available for use. The
 * hook is executed before listing tests and before running tests.
 *
 * @return
 *   The test case class name related to the item should be specified in the
 *   array key.
 *   - "name": Required. The translated formal name of the test.
 *   - "description": Required. The translated description of what the test does.
 *   - "group": Required. The translated group name to categorize the test in.
 *   - "file": The name of the file the test in located in, excluding the .test
 *     extension. Deaults to MODULENAME.test.
 *   - "file_path": The path to the file if not located in the root of the
 *     module directory.
 */
function hook_test() {
  $tests = array();

  $tests['ExampleTestCase'] = array(
    'name' => t('Example'),
    'description' => t('Tests example stuff.'),
    'group' => t('Example'),
  );
  $tests['AdditionalExampleTestCase'] = array(
    'name' => t('Additional example'),
    'description' => t('Tests additional example stuff.'),
    'group' => t('Example'),
    'file' => 'additional_example',
    'file_path' => drupal_get_path('module', 'example') . '/tests',
  );

  return $tests;
}

Redesigned per my previous idea.

/**
 * Define tests provided.
 *
 * This hook allows modules to add tests to global list available for use. The
 * hook is executed before listing tests and before running tests.
 *
 * @return
 *   The test case class name related to the item should be specified in the
 *   array key.
 *   - "name": Required. The translated formal name of the test.
 *   - "description": Required. The translated description of what the test does.
 *   - "group": Required. The translated group name to categorize the test in.
 *   - "file": The name of the file the test case is located in, excluding the
 *     test extension. If specified the file should be located in a tests
 *     folder in the module root, /path/to/modules/tests. Deaults to
 *     MODULENAME.test.
 */
function hook_test() {
  $tests = array();

  $tests['ExampleTestCase'] = array(
    'name' => t('Example'),
    'description' => t('Tests example stuff.'),
    'group' => t('Example'),
  );

  // File additional_example.test found in /path/to/module/tests directory.
  $tests['AdditionalExampleTestCase'] = array(
    'name' => t('Additional example'),
    'description' => t('Tests additional example stuff.'),
    'group' => t('Example'),
    'file' => 'additional_example',
  );

  return $tests;
}

simpletest_get_all_tests() is much cleaner now!

I would like to do further cleanup of the simpletest.module so that run-tests.sh isn't as independent.

This should work for the simpletest.test only. I would like to confirm the implementation before I take the time to convert all the other getInfo() implementations to hook_test().

The first item is copied directly out of menu.inc :) [will change]
Second [will change]
The escaping was there before I just copied it out of getInfo(), but I can change it. [will change]

Per #7.

What I need now is help rewriting all the getInfo() as hooks :)

Way more test cases then I was thinking (suppose I could go out to t.d.o and see that)...Got a bunch...but by no means done...I have to work on something else...and d.o will be done shortly...I'll see if I can't come up with a script.

EDIT: This patch file was overridden...this is equal to #14.

Made script to do most of the work for me...then painstakingly double checked everything.

require_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'simpletest') . '/drupal_web_test_case.php';
$files = file_scan_directory('.', '/.*.test$/m');
//print_r($files);
$classes = array();
foreach ($files as $file) {
  $existing_classes = get_declared_classes();
  require_once DRUPAL_ROOT . '/' .$file->filename;
  $new_classes = array_values(array_diff(get_declared_classes(), $existing_classes));
  
  $parts = explode('/', $file->filename);
  $module = $parts[2];
  
  if (!isset($classes[$module])) {
    $classes[$module] = array();
  }
  
  if (strpos($file->filename, '/tests/') !== FALSE) {
    // In subfolder.
    $classes[$module]['tests'][$file->name] = $new_classes;
  }
  else {
    $classes[$module]['root'] = $new_classes;
  }
}

//print_r($classes);
ksort($classes);

foreach ($classes as $module => $module_tests) {
  $info = array();
  foreach ($module_tests['root'] as $test_class) {
    $instance = new $test_class;
    if (method_exists($instance, 'getInfo')) {
      $info[$test_class] = $instance->getInfo();
    }
  }
  
  foreach ($module_tests['tests'] as $file => $tests) {
    foreach ($tests as $test_class) {
      $instance = new $test_class;
      if (method_exists($instance, 'getInfo')) {
        $info[$test_class] = $instance->getInfo();
        $info[$test_class]['file'] = $file;
      }
    }
  }
  
  echo "Module: $module\n";
  echo "------------------------\n";
  
  echo "/**\n * Implementation of hook_test().\n */\n";
  echo "function {$module}_test() {\n" . '  $tests = array();' . "\n";
  foreach ($info as $test_class => $info) {
    $array = var_export($info, TRUE);
    
    $array = str_replace('array (', 'array(', $array);
    $array = str_replace(" => \n  array(", " => array(", $array);
    
    $array = str_replace("' => '", "' => t('", $array);
    $array = str_replace("',", "'),", $array);
    $array = preg_replace("/'file' => t\((.*?)\),/", "'file' => $1,", $array);
    echo str_replace("\n", "\n  ", "\n" . '$tests[\'' . $test_class . '\'] = ' . $array . ";");
  }
  
  echo "\n\n  return \$tests;";
  echo "\n}";
  echo "\n\n";
}

return;
ksort($files);
foreach ($files as $file) {
  $contents = file_get_contents($file->filename);
  $contents = preg_replace("/\n\s+function getInfo\(\) {.*?}/s", '', $contents);
  
  if ($contents) {
    file_put_contents($file->filename, $contents);
    echo $file->filename . "\n";
  }
}

return;

Note: during the bugged file upload state this file overrode the previous one. So the comments and the test results may not make a lot of sense. What test results you see will be for the file above. (another bug I can't edit the text of #12)

Correcting issue...updating patch, with overriding file from last night.

Previous patch used module_implements() which ignores modules that are disabled. So the following tests did not run. New patch uses module_rebuild_cache() instead.

Array
(
    [36] => Search engine ranking
    [37] => Advanced search form
    [38] => Bike shed
    [39] => Search engine queries
    [41] => Autocompletion
    [42] => Test field weights
    [43] => Test date field
    [44] => Test select field
    [45] => Test single fields
    [46] => Poll add choice
    [48] => Poll vote
    [49] => Poll create
    [50] => PHP filter access check
    [51] => PHP filter functionality
    [52] => Path aliases with translated nodes
    [53] => Path alias functionality
    [66] => Language switching
    [67] => Locale uninstall (FR)
    [68] => Locale uninstall (EN)
    [69] => Translation import
    [70] => String translate and validate
    [72] => Forum functionality
    [75] => Field SQL storage tests
    [108] => Personal contact form
    [109] => Site-wide contact form
    [116] => Book functionality
    [117] => Blog API functionality
    [118] => Blog functionality
    [121] => Import feeds from OPML functionality
    [122] => Categorize feed item functionality
    [123] => Remove feed item functionality
    [124] => Update feed item functionality
    [125] => Remove feed functionality
    [126] => Update feed functionality
    [127] => Add feed functionality
    [146] => Top visitor blocking
    [147] => Syslog functionality
    [169] => Text Field
    [170] => Tracker
    [171] => Translation functionality
    [172] => Trigger content (node) actions
    [173] => Upload functionality
)

Due to weird bug last night...results for #15 are displayed on #10.

chx recommended not use module_invoke.

After talking in IRC we decided to move the hook_test() to the .test files for several reasons:

• Disabled modules will not have a registry to locate the hook for us so we need a common place to find it.
• The test file provides a consistent place to find and goes along with pattern of breaking up code.
• Doesn't require loading of disabled .module files. ("dead code" -chx)
• Still allows the use of a hook and doesn't require instantiating the test classes.

The previous version didn't work because SimpleTest didn't locate it's hook in a standard file and thus caused the problem with the removal of module_invoke() which lead us to realize that the problem was worse for disabled modules.

The reason this issue has died was after extensive discussion in IRC, I will try to explain all the possibilities and reasons.

• Having all the test information in a hook requires that the test case name is typed exactly and separates the information from the actual test.
• PHP does not support dynamic calling of static methods until until PHP 5.3.0 which Drupal will not require.
• Hooks are intended to all be called on enabled modules, what that means is that only tests from enabled modules will display. I personally prefer that from the interface standpoint, but there needs to be a clean way to run all tests. If they are in hooks it would require either enabling of all modules or forcefully including all .test files and forcing the hook_test() to be there.

Possible solutions

• Leave as it is and add a parameter to the test case constructor that is a flag that determines if the test case is being actually run or just retrieving info.
• Require PHP 5.3.0 for SimpleTest and implement static getInfo methods once released.
• Only allow running tests for enabled modules.
• Something better that someone is waiting to suggest.

#21: I think that is the best solution, great idea!

+1

To see kinda how we are using the php doc block look at Java's annotations. You can see they made it a language construct (modifier).

• http://java.sun.com/j2se/1.5.0/docs/guide/language/annotations.html
• http://www.developer.com/java/other/article.php/3556176
• http://en.wikipedia.org/wiki/Java_annotation

Theoretically this type of registry could be used to do some cool stuff (also simplify API modules job :)).

Anywhere you just want to register function as being some sort of hook/callback and with some meta data these can be used.

I think in SimpleTest's case and the ones DamZ mentioned in the other issue it is the best solution. Heres a before and over of what it will look like.

Before

After

The advantages being:

• The test information is kept near the test class.
• We force documentation of test classes!
• It looks nice and is simple.
• No chance for type that will cause test to not show up (like hook_test())
• Does not require instantiating all the test classes just to get the information. (Proper code) Thus no need for __constuctor($info = FALSE) flag to differentiate.
• Elevates the file loading that SimpleTest does and moves to to registry like the rest of core.
• Open up future possibilities and DamZ's use case.

After our discuss in IRC I updated the script (attached), ran it and manually scanned the changes, and merged the simpletest.module portion of the patch.

Need to compare total assertion count from testbot to ensure we got all tests picked up.

Forgot changes to runtests.sh and simpletest.api documentation.

Commit of #259368: Allow Inline CSS in drupal_add_css caused failure to apply.

Reroll.

As I've stated in #27 I agree that annotations are not new, but:

If we are going to make the system auto detect and use test classes then wouldn't it make sense to do the same with hook_theme(), hook_menu(), etc, which is a major shift which was the idea behind discussing that in the related issue. My impression was that this wasn't a "no", but more "we need more people to look this over and update core to make it consistent if we decide to do it".

From that standpoint if we make this a hook that solves our problem now and if we so choose to make the shift then hook_test() can change along side the others.

NOTE: I found a one line change that needed to be made to the above patch. I'd like it to be tested and the results reported back, can we leave the issue as needs review so the results will be recorded. The previous patch results look like it worked properly, but the way PIFT functions they are ignored when issue isn't a valid status.

Interestingly enough the PHP documentation was incorrect (or at least what I read) and you can infact invoke a static method using a dynamic class name. (thanks Damien) I remember to try it anyway and ignore documentation next time.

class Something {
  public static function getInfo() {
    return 'hax';
  }
}

$class = 'Something';
$method = new ReflectionMethod($class, 'getInfo');
echo $method->invoke(NULL);

// Result: hax

I am working on a patch right now.

Here is the updated script used.

require_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'simpletest') . '/drupal_web_test_case.php';
$files = file_scan_directory('.', '/.*.test$/m');
foreach ($files as $file) {
  $contents = file_get_contents($file->filepath);
  $contents = preg_replace("/function getInfo\(\)/", 'public static function getInfo()', $contents);
 
  if ($contents) {
    file_put_contents($file->filepath, $contents);
    echo "Change getInfo() to static in: $file->filename...\n";
  }
}

Somehow patch didn't get the individual test changes.

Since the inner workings of run-tests.sh and the web runner do not share common API elements (I want to fix, but at the moment I am focusing on PIFT/PIFR) I forgot to update the script.

Changed:

$method = new ReflectionMethod($class, 'getInfo');
$info = $method->invoke(NULL);

to:

$info = call_user_func(array($class, 'getInfo'));

With the improvements I have planned I might be able to get that down a bit more, mainly focused on code cleanup though.

Upgrade documentation? Simpletest 6.x-2.x should be where people are coming from and I backport core regularly so it should have this soon.

Thanks for the time you spent discussing this.

Well the idea is they can keep up with core improvements...obviously if they don't want to they stick with a previous version, but especially for people coming onto the scene it is nice.

Also I don't develop SimpleTest 6.x-2.x at all, if people have feature requests I move them to core. It doesn't make a lot of sense to maintain two separate pieces of code. Majority of the backports haven't messed with API, only added to it or corrected it.

Checked all pages and updated

• http://drupal.org/node/325974
• http://drupal.org/node/30021
• http://drupal.org/node/273612
• http://drupal.org/node/30008