Posted by boombatower on February 18, 2009 at 6:04am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | simpletest.module |
| Category: | task |
| Priority: | normal |
| Assigned: | boombatower |
| Status: | closed (fixed) |
| Issue tags: | DX (Developer Experience), Needs Documentation |
Issue Summary
Reasons:
- Scanning filesystem instead of using a hook is un-drupalish. (DX)
- Instantiating all test cases just to call getInfo() is incorrect code usage.
- Blocker for #323477: Increase simpletest speed by running on a simplified profile
Changes:
- Create a hook_test() to provide the information getInfo() does.
- Update core tests to reflect it
- Implement in SimpleTest runner and run-test.sh.
Comments
#1
This is a good idea. Subscribing to see how it might affect #343502: Allow tests to require and test existance of contrib modules.
#2
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:
<?php
/**
* 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;
}
?>
#3
Redesigned per my previous idea.
<?php
/**
* 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;
}
?>
#4
Very nice. that getInfo business was really ick, agreed totaly on the original post and the suggested hook.
#5
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().
#6
#7
if (isset($test_classes) && is_array($test_classes))simplify toif ($test_classes). In simpletest_load_test move(isset($info['file']) ? 'tests/' . $info['file'] : $info['module'])into a variable (like $basename), 'cos it's hard to read. Finally, in your new file, do not hesitate to use double quotes to be able to use unescaped single quotes. Escaped single quotes are ugly.#8
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]
#9
Per #7.
What I need now is help rewriting all the getInfo() as hooks :)
#10
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.
#11
Made script to do most of the work for me...then painstakingly double checked everything.
<?php
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;
?>
#13
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)
#14
Correcting issue...updating patch, with overriding file from last night.
#15
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
)
#16
Due to weird bug last night...results for #15 are displayed on #10.
#17
chx recommended not use module_invoke.
#18
After talking in IRC we decided to move the hook_test() to the .test files for several reasons:
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.
#19
My "dead code" remark was referring to putting hook_test in .module files as these are not needed for normal module work. Also, furthering the original argument, Instantiating all test cases just to call getInfo() is incorrect code usage. -- indeed, calling the constructor for two, enitirely different reasons is simply wrong. A constructor should be able to set up something for all of its test cases, finally providing a meaningful answer to the question, which test functions belong to one class?
#20
The last submitted patch failed testing.
#21
Building above #393068: Make the registry class hierarchy, interface and docblock aware, this patch implement direct parsing of docblocks. I'm really against creating yet-another-registry-hook. Plus, this removes the "dead code" concern.
Note: only two tests were converted to the new info docblock yet.
#22
Based on the patch above, I believe it will be relatively easy to implement a fully clean environment (based on the idea of implementing a separate test-runner.php in the root directory, and bootstrapping the target environment from that... that whole idea requiring us to now which file a given test class is defined in, which is impossible both in the current situation).
#23
The last submitted patch failed testing.
#24
The registry allows hook_test() implementations to be put in any file, so it will never be loaded unless it is actually needed.
#25
The reason this issue has died was after extensive discussion in IRC, I will try to explain all the possibilities and reasons.
Possible solutions
#26
#21: I think that is the best solution, great idea!
+1
#27
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).
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:
#28
I'm pretty -4000 to embedding important registration info in docblock properties from a DX perspective:
a) Everywhere else I want to "register" something with Drupal, I implement a hook (hook_perm, hook_menu, hook_theme, etc.). This is deviating from the standard that, for better or worse, currently exists.
b) While writing Doxygen comments is /highly recommended/ it is not /required/. Moving to this model would make it required, and also introduce a special Drupal-only syntax for it.
We can discuss this more in #393068: Make the registry class hierarchy, interface and docblock aware about whether or not it's worth moving all registry hooks to this model, but IMO it's a bad idea to hitch the future of this patch on that one.
Implementing hook_test() in the .test file (so that it can be scanned even in disabled modules) seems like the only way forward here.
#29
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.
#30
Forgot changes to runtests.sh and simpletest.api documentation.
#31
The last submitted patch failed testing.
#32
Commit of #259368: Allow Inline CSS in drupal_add_css caused failure to apply.
Reroll.
#33
I'm pretty -4000 in hook_test_info() from a DX perspective:
- you already have created your test class, extended DrupalWebTestCase, you gave it a name, there should be nothing more required from you
- in particular, you don't want to declare your intention twice (first by creating the class, then by registering the class in hook_test_info())
- the system could be smart enough to deal with empty title, description and group: by default, take the name of the class as the title, an empty description, and assign the test to the "Other" group.
So -4000 for yet-another-registry-we-don't-need:
- we already have a registry of classes and interfaces
- as I demonstrated in #393068: Make the registry class hierarchy, interface and docblock aware, it's easy enough to extend it so that it also read meta-data about those
- this method is used elsewhere, most notably in Java / J2EE under the name of 'annotations', so it's not at all a "Drupalism", even if the keywords we will implement are obviously specific
#34
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.
#35
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.
<?php
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.
#36
Here is the updated script used.
<?php
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";
}
}
?>
#37
Somehow patch didn't get the individual test changes.
#38
The last submitted patch failed testing.
#39
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.
#40
#41
Changed:
<?php$method = new ReflectionMethod($class, 'getInfo');
$info = $method->invoke(NULL);
?>
to:
<?php$info = call_user_func(array($class, 'getInfo'));
?>
#42
that's a pretty workable solution. Although it needs the test files but at least we do not instance the classes needlessly. This is really a staple case for static methods -- getting static data out of the class -- so static it's constant :) Nice job.
#43
Did some devel.module memory profiling for admin/build/testing and confirmed some basic testing I had done.
Before patch:
Memory used at: devel_boot()=1.22 MB, devel_shutdown()=21.44 MB, PHP peak usage=23.25 MB.
After patch:
Memory used at: devel_boot()=1.22 MB, devel_shutdown()=20.98 MB, PHP peak usage=22.75 MB.
Yay! :)
#44
With the improvements I have planned I might be able to get that down a bit more, mainly focused on code cleanup though.
#45
Great. :) Committed to HEAD!
Needs to be reflected in the SimpleTest docs and the upgrade documentation.
#46
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.
#47
Yikes! You shift the API under contrib developers' feet like that?! :) Ok, your perogative...
But we at least need it updated in places like http://drupal.org/node/325974 and http://drupal.org/node/30021 and so on. Everything under http://drupal.org/simpletest could probably use a quick pass.
#48
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.
#49
Checked all pages and updated
#50
Automatically closed -- issue fixed for 2 weeks with no activity.