Posted by catch on August 10, 2008 at 4:02pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Needs tests |
Issue Summary
Needs a test: http://api.drupal.org/api/function/menu_set_item/7
Comments
#1
Hi,
Yusuf and I created a simple test for this function as part of our companies OpenAfternoon project. This code needs reviewing.
#2
All tests pass.
#3
Ok, since this is a testingparty patch, I'm going to be merciless, because that helps you learn. Also, it's fun. :) Pay it forward!
<?phpIndex: modules/simpletest/tests/menu_set_item.test
?>
I would go ahead and add this to the existing menu.inc tests, which are in modules/simpletest/tests/menu.test.
<?php+class menuSetItemTest extends DrupalWebTestCase {
?>
a) This line needs a sentence of PHPDoc above it explaining what's going on.
b) The class should be capitalized.
c) Also needs to be MenuSetItemTestCase.
<?php+ function getInfo() {
?>
Please preface this function with PHPDoc that says "Implementation of getInfo()."
<?php+ 'name' => t('set menu item testcase'),
?>
a) Capitalize Set.
b) I would just call this 'Tests for menu_set_item()' since that's a little more clear.
<?php+ 'description' => t('Test menu_set_item functionality.'),
?>
Hm. Not sure that description's very useful. :) Could you be more specific? What does menu_set_item() do that's being tested?
<?php+ function testMenuSetItem() {
?>
PHPDoc please. :)
<?php+ $orig_item = menu_get_item('node');
?>
We don't abbreviate variable names. Should be $original_item. Needs to be changed throughout.
<?php+ $this->assertEqual($orig_item['path'], 'node', t('Path from menu_get_item(\'node\') is equal to \'node\''), 'menu');
?>
Let's simply put that string in double quotes so you don't need all the escaping.
<?php+ $orig_item['path'] = 'foobar';
+ $orig_item['href'] = 'foobar';
+ $orig_item['title'] = 'view foobar';
?>
Could we come up with values here that are a little more meaningful? Like can you think of a legitimate reason one might be changing these properties, or are you literally just throwing random stuff at it to see how it reacts?
<?php+ $test_item = menu_set_item('node',$orig_item);
?>
Need a space after the comma.
<?php+ $this->assertEqual($orig_item, $compare_item, t('Modified item is equal to newly gotten item'), 'menu');
?>
Gotten isn't really a word. Let's try retrieved.
That's all I could find. :) Thanks a lot!!
#4
Ouch ! ... you sure are harsh, but ... okay, you were right in most cases.
The things about the random stuff is true but there are not that many cases in which I could find a use for this function, I could not even find a place where it was used in the whole Drupal code, but eventually we came up with some form use for it.
Here's the new patch, hope you like it ;)
#5
The inline code comments in this patch are not according to the Drupal conventions. Prefix with
//and terminate your comments with a point. Almost there, ThiOz! :)#6
Finally I got some time to fix these minor issues ,
here's the new patch
C Ya
#7
Hi
the patch made by eclipse wasn't made correctly so this one I made through the commandline
cheers
#8
Patch is empty :(
#9
The new patch for menu_set_item()
#10
Hi Wisif, you're missing a space in "modified for testing purposes,we "
Also, since core testing creates a completely clean drupal install and removes it once finished there's no need for 'Test:' prefixes. Otherwise this looks great. Marking to needs review to run it past the test bot.
#11
The last submitted patch failed testing.
#12
i rerolled the patch and changed the stuff from here http://drupal.org/node/293511#comment-1150200
#13
The prefix mention should be removed from the inline comment as well.
"// To make clear this is a testnode, we prefix the title."
#14
thx for review,
i removed the previous comment and added a better description for the assert
#15
Spotted a couple more nitpicks, sorry this is so piecemeal, rather than marking CNW I've re-rolled with the changes.
#16
#17
Even smaller nitpick: we should be using single quotes in the second assertion, no need to use double quotes. Patch rerolled accordingly.
#18
Yay more tests!
#19
Committed to CVS HEAD. Thanks.
#20
Automatically closed -- issue fixed for 2 weeks with no activity.