TestingParty08: menu_set_item().

Hi,

Yusuf and I created a simple test for this function as part of our companies OpenAfternoon project. This code needs reviewing.

All tests pass.

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!

Index: 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.

+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.

+  function getInfo() {

Please preface this function with PHPDoc that says "Implementation of getInfo()."

+      '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.

+      '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?

+  function testMenuSetItem() {

PHPDoc please. :)

+    $orig_item = menu_get_item('node');

We don't abbreviate variable names. Should be $original_item. Needs to be changed throughout.

+    $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.

+    $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?

+    $test_item = menu_set_item('node',$orig_item);

Need a space after the comma.

+    $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!!

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 ;)

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! :)

Finally I got some time to fix these minor issues ,

here's the new patch

C Ya

Hi

the patch made by eclipse wasn't made correctly so this one I made through the commandline

cheers

Patch is empty :(

The new patch for menu_set_item()

i rerolled the patch and changed the stuff from here http://drupal.org/node/293511#comment-1150200

thx for review,

i removed the previous comment and added a better description for the assert

Even smaller nitpick: we should be using single quotes in the second assertion, no need to use double quotes. Patch rerolled accordingly.

Yay more tests!

Committed to CVS HEAD. Thanks.