Posted by catch on August 10, 2008 at 4:04pm
7 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | tests |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Need to test menu rebuilding through the variable 'menu_rebuild_needed'.
Comments
#1
Harold (MadHarold) and I wrote a simple test for this which should do the job. The test tests if the rebuild is fired by manually deleting a record from menu_router and checking if it returns after a page request.
See patch attached.
#2
All tests pass.
#3
Change description of final assertion, change delete query to use DBTNG.
#4
It works and the code makes sense.
#5
Ok, since this is a TestingParty patch, that means a merciless review since that way you learn best practices. Also, it's fun. ;)
<?php+class MenuRebuildCase extends DrupalWebTestCase {
?>
a) That should be MenuRebuildTestCase.
b) Please include a one-liner of PHPDoc to explain what this test case is for.
<?php+ /**
+ * Test if the 'menu_rebuild_needed' variable triggers
+ * an menu_rebuild() call.
+ */
?>
a) PHPDoc above functions should always attempt to fit in one line at < 80 chars. If more explanation than that is needed, make it a second sentence below.
b) Should be a menu_rebuild() not an menu_rebuild().
<?php+ // Check if 'admin' exists
?>
All comments should end in a period.
<?php+ $adminexists = db_result(db_query("SELECT path from {menu_router} WHERE path='admin'"));
?>
a) We don't smoosh together words, so this should be $admin_exists. Needs changing throughout.
<?php+ $this->drupalGet('<front>');// menu_execute_active_handler() should trigger the rebuild.
?>
Please move this to up above this line instead. Inline inline comments are ok for rare circumstances but are generally discouraged, especially when they're long like this.
Darn! That's all I could find. Great job! :)
#6
Fun huh? I can't imagine what you qualify as 'unfun,' but anyway....
Fixed all your points.
#7
Updated patch to remove the always surprising trailing whitespace.
#8
The last submitted patch failed testing.
#9
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#10
We no longer have phpdoc for setUp(), getInfo() or tearDown()
http://drupal.org/node/325974
#11
Something to be thankful for, at a thankful time of year ;)
Cleans up all of modules/simpletest/tests/menu.test.
#12
Patch looks good, all tests pass. RTBC.
#13
Committed to HEAD with some minor whitespace clean-up. Thanks!! :)
#14
Automatically closed -- issue fixed for two weeks with no activity.