TestingParty08: menu router item titles need a test

I am taking a look at this test this week :)

Here is a patch that tests the 4 cases in http://drupal.org/node/140311 and uses string overrides. I had to move a comment in the existing code in menu_test.module to make hook menu still make sense. I also deleted a (wrong) call to t() in a menu item.

Also, i think this test also tests #293515: TestingParty08: test 'title arguments' using t() - the test needed is the same

i don't like urls to issues in the documentation of a certain function

+   * Tests the 4 possible ways to set the title for menu items mentioned in:
+   * http://drupal.org/node/140311 Also tests that menu item titles work 
+   * with string overrides http://drupal.org/node/131061
+   */

some codestyles changed in drupal7

i changed the patch ...

there should be "@" instead of "!" placeholders, because there is not html behind the placeholder
Added documenation to the helper function, but i don't understand why 3 is the default value of case_number

i changed to

added a patch

Thanks for the quick review :) I made a patch with some corrections to the doc-comment to menu_test_title_callback(). It does not use placeholders, so I changed the text.

I agree that the default 3 is a little unclear. It is to test a menu item that uses a title callback, but no title arguments (case 3). I thought that both case 3 and case 4 could use it that way.

Rerolled the patch - it did not apply

Here is the patch rolled against the latest version. Hope it works.

Rolled another

Woops. And attaching the patch

Patch looks good. It applied and ran fine for me. I noticed the Group for the test results shows up as 'Other'. There's an optional parameter you could set (I assume to 'menu') for calls to assertTitle().

Thanks - fixed the group and made a function private

one short stuff

the sepperation between strings in drupal7 is like this

<?php
$foo . 'bar';
?>

I applied the new patch and ran the menu test again. Works fine, looks good.

OK, I changed the string concatenation to the standard

Oh.. Sorry. And forgot to remove the .txt..

     $this->assertEqual($name, 'changed', t('Menu name was successfully changed after rebuild.'));
   }
+  
+  /**

I really have a knack for hunting down that trailing whitespace today. :)

Tests the possible ways to set the title for menu items should end in a period.

$asserted_title . ' | Drupal' That seems to hard-code the theme layer... is this okay here?

   );
+ 
+  $items['menu_title_test/case1'] = array(
...
+    'page callback' => 'user_page', 

More whitespace (the latter line multiple times).

Concatenates a string, by using the t function and a case number.

When referring to a function in Doxygen comments, always use t() - this will cause the API generator to link to the function.

+  return t($title) . ' - Case ' . $case_no; 

More whitespace, yay! :)

Also, "Defaults to 3" should be in parentheses rather than a separate sentence, for consistency.

Thanks Arancaytar :)
I removed whitespace and added some periods.
About $asserted_title . ' | Drupal', I think it is OK for tests.

so just a few more tiny things.

Every comment is a sentence, so it should end with an "."

@param uses a new line for the description text.

this should have needs review

I don't understand. I looked over every line of these two patches and did not see any difference that could have explained the passing of the first and the failing of the latter.

Reroll.

When applying the patch from #22, there were a few lines missing in menu_test.module, so a function disappeared.

@dereine If you edit a patch instead of generating it, please take care when adding lines - it breaks the patch. The safer way is to apply the patch, make changes, and make a new patch.

If you edit a patch instead of generating it, please take care when adding lines

Ah, yes. I've tried that occasionally because it seemed so deceptively simple - but ultimately, the line numbers and offsets bit me. This patch format is more complex than it seems, and while it is human-readable very easily, it is not human-writable by any means.

yes i edit just the patch :( my fault

+   * the title is asserted to begin with "Alternative"

is missing the .

And:

 @param $title 
+ *  Title string.
+ * @param $case_number 
+ *  The current case number which is tests (defaults to 3).

should be

+ * @param $title 
+ *   Title string.
+ * @param $case_number 
+ *   The current case number which is tests (defaults to 3).
+ */

Corrected indentation and punctuation :)

Reroll

Looks good to me now.

URLs should use dashes instead of underscores in Drupal.

Replaced the underscores with dashes.

Also added an (empty) context key to the locale_custom_strings_en to keep up with #334283: Add msgctxt-type context to t()

Setting to previous status - testbot was broken (failed to install).

Reroll. I also changed a confusing call to $this->randomName() in a .module file to an ordinary static string.

HEAD is broken.

Marking as need review to retest - I cannot make the test fail.

Reroll

Sorry, but tests don't qualify as critical anymore.

+++ modules/simpletest/tests/menu.test	29 Dec 2009 21:11:43 -0000
@@ -354,6 +354,39 @@ class MenuRouterTestCase extends DrupalW
+  /**
+  * Tests the possible ways to set the title for menu items.
+  * Also tests that menu item titles work with string overrides.
+  */

code style issue: the 3 last lines should be indented one space, so the asterisks are aligned. Same goes for the other comments.

+++ modules/simpletest/tests/menu.test	29 Dec 2009 21:11:43 -0000
@@ -354,6 +354,39 @@ class MenuRouterTestCase extends DrupalW
+  ¶

trailing whitespace

+++ modules/simpletest/tests/menu_test.module	29 Dec 2009 21:11:44 -0000
@@ -24,6 +24,34 @@ function menu_test_menu() {
+  ¶

trailing whitespace

Looks good from a functionality perspective.

Reroll with the changes from #50 :)

Testbot likes it, code looks good.

#51: 296115-51.patch queued for re-testing.

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

Mmmm. Tests. I love tests!

Committed to HEAD. Thanks!