Posted by boombatower on April 30, 2009 at 2:29am
8 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | simpletest.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | boombatower |
| Status: | closed (fixed) |
| Issue tags: | documentation, Registry |
Issue Summary
This is part of my continued clean-up of SimpleTest and its API.
One area that really needs some attention is simpletest_get_all_tests() and the related uses. The getInfo() method is called way to often and should be called centrally.
Also I am looking into having the registry auto-load tests and provide SimpleTest with a list.
Comments
#1
Well, you can add .test files to .info and then you can fetch that information
select * from registry where type='class' and filename like '%.test';to find candidates and then run amethod_exists($object->name, 'getInfo')to find whether they contain the necessary method.#2
The
simpletest_get_all_tests()function is called 7 time and in each case it is used to auto-load classes or get the getInfo() information as you would expect.It might almost make sense to just cache the getInfo() since registry will take care of auto-loading...then we have no need to even load all the test files 90% of the time.
Created separate issue, but may end up merging: #449200: SimpleTest: Cache test information.
#3
It takes 4 lines to cache, so I am just going to include it in this patch I'll mark other as duplicate.
This is extremely rough code since it only contains an updated
simpletest_test_get_all()(also renamed) and does not change the code where called. I think it is best to wait for #445950: SimpleTest interface separation and cleanup since it will change how much of the calling code works.#4
Other issue committed. I will begin work on this again.
#5
Seems to run well and should be faster since everything is cached and we no longer have to load all test files every time we want information on one or all.
Removes 4 calls to
simpletest_test_get_all()and lazy-loads test files instead of loading them all whenever it needs one! The code was looping over the same stuff numerous times in the same request...it no longer does that.This patch does not the addition of test files to all the .info files. I'll get that next.
#6
This should be full version of patch.
#7
These are the tests that are missing:
Array(
[0] => Add feed functionality
[1] => Advanced search form
[2] => Autocompletion
[3] => Bike shed
[4] => Blog API functionality
[5] => Blog functionality
[6] => Book functionality
[7] => Categorize feed item functionality
[8] => Comment Search tests
[9] => Content language settings
[10] => Forum functionality
[11] => Import feeds from OPML functionality
[12] => Language configuration
[13] => Language switching
[14] => Locale uninstall (EN)
[15] => Locale uninstall (FR)
[16] => OpenID helper functions
[17] => OpenID login and account registration
[18] => Path alias functionality
[19] => Path aliases with translated nodes
[20] => Path language settings
[21] => Personal contact form
[22] => PHP filter access check
[23] => PHP filter functionality
[24] => Poll add choice
[25] => Poll create
[26] => Poll vote
[27] => Remove feed functionality
[28] => Remove feed item functionality
[29] => Search engine queries
[30] => Search engine ranking
[31] => Site-wide contact form
[32] => String translate, search and validate
[33] => Syslog functionality
[34] => Test date field
[35] => Test field weights
[36] => Test select field
[37] => Test single fields
[38] => Text Field
[39] => Top visitor blocking
[40] => Tracker
[41] => Translation export
[42] => Translation functionality
[43] => Translation import
[44] => Trigger content (node) actions
[45] => Update feed functionality
[46] => Update feed item functionality
[47] => Upload functionality
[48] => User language settings
)
Not sure why since for instance aggregator tests are not in list, but there is an entry in the .info file. Also registry does not even have a record for aggregator.test. Looks like might be related to registry.
#8
Hmm....slip of the brain....this was the issue over at hook_test()....it only picks up enabled modules. Hmm
That means there are two options:
Continue to use registry, but only display enabled modules and come up with something special (or enable all modules) for t.d.o.
I would really like to lazy load the tests...much better for performance and cleaner to implement.
#10
#11
Included necessary registry changes and new tests in test folder: batch and error.
#12
The last submitted patch failed testing.
#13
Get a single exception in the graph test.
If I change the class to DrupalWebTestCase instead of DrupalUnitTestCase it works fine.
There are 7 other test cases that use the DrupalUnitTestCase and still pass.
#14
The following change fixed it, as one would expect. I am not entirely sure why this only manifests itself with the patch.
<?phprequire_once 'includes/graph.inc';
// drupal_function_exists('drupal_depth_first_search');
?>
#15
All inclusive patch.
#16
I think caching is fine, but only if it is worth the trouble. Benchmark results can tell us that. If there are no significant improvements, it's probably not worth it?
Other than that, this looks like a fine clean-up to me.
#17
There are a two reasons for caching:
1) Avoid loading all the .test files and calling all the getInfo() methods in the 100+ test cases every page load and during test running with the patch API (so 100+ times on t.d.o).
2) Avoid having to perform sorting and grouping of array every page load. (there are several patches which will add a bit more information to the array)
I have not done a benchmark, but I would bet it is worth it. Anyone want to run a benchmark, otherwise I can spend some time and learn how.
#18
Subscribing (#481498: Update simpletest module to use drupal_static() is postponed on account of this issue.)
#19
I am in love with the patch. It's hard not to when you are removing so much and let the registry do the work which it already does anyways. Two small problems:
Parse .info file only for all modules <= Huh??
if (preg_match('/.*?\.test/', $file)) { <= substr($file, 0, -5) works. I am aware of the powers of preg but that's overkill here (also if you were to use preg, you would need simple /\.test$/ here)
#20
Seems from #19 that would be a solid review, thus marking as RTBC. thx
Changed comment in question to:
// Parse the .info file for all modules, reguardless of their status so the// list of files can then be used in hook_registry_files_alter()
// implementations.
and condition in question should be:
<?phpsubstr($file, -5) == '.test'
?>
#21
- "necessary alterations" -- it would be good to describe what these alternations are.
-
File provided by disabled modules-- should be "Files".-
an array of the related module name and weight-- should be "modules".- The section on
+ * @param $filesseems to need some work -- it doesn't read well.-
contains an property-- should be "a property".-
where is looks-- should be "where it looks".- Etc.
The documentation needs some more TLC and a good scrub!
#22
"an array of the related module name and weight" refers to (per documentation):
<?php/* ...
* @code
* $files["modules/system/system.module"] = array(
* 'module' => 'system',
* 'weight' => 0,
* );
* @endcode
* ...
*/
?>
which is the related module and weight.
Updated for #429132: Remove unecessary module_rebuild_cache() call from _registry_rebuild().
Added additional documentation in several places and clarified others.
All other changes made.
#23
Committed to CVS HEAD. Thanks.
#24
clean HEAD, enable simpletest module:
* Notice: Undefined index: files in simpletest_registry_files_alter() (line 281 of /home/catch/www/7/modules/simpletest/simpletest.module).* Warning: Invalid argument supplied for foreach() in simpletest_registry_files_alter() (line 281 of /home/catch/www/7/modules/simpletest/simpletest.module).
#26
confirming exact same errors on a fresh install.
#27
This code just revealed a bug introduced by other patch: http://drupal.org/node/147000#comment-1675232. Already talked with Berdir in IRC.
#28
docs only update.
i went in looking to change the comment about installed modules, as the registry is now full of files and resources for uninstalled modules. i also found we had some outdated caching comments at the top of the file, so they can go as well.
#29
Committed to CVS HEAD. Thanks.
#30
Automatically closed -- issue fixed for 2 weeks with no activity.