Menu access callbacks need a test

howto write the first test

Create a custom Module (in info set hidden = True)
implement the hook_menu and have it call an access_callback function.
Provide some content to be displayed when you would 'click' the menu

Have the access_callback return TRUE.

From within the menuCallback.test ..
call the menu page.
Check whether the result returns your own page.

Next...

have another access_callback return FALSE

From within the menuCallback.test ..
call that menu page.
Check that it returns a 404 (page not found)

Also check that $access = false
....

---------

Sorry, I've wanted to do this, but all the time stuff getting between it.

Wouldn't it return 403 instead of 404 as the menu item exists, you just do not have access permissions?

I could also see this test and other callback related items within the test file created at #331720: TestingParty: Write a test for menu callbacks in different files

subscribe so I rememebr to review

A new patch! Adds code from #296293: TestingParty08: menu access callback default need a test (mikey_p)

Currently does not take care of "Define local tasks below and check MENU_DEFAULT_LOCAL_TASK properly inherits." in the issue description.

Fixed a menu test that was comparing the results of drupalGet. Now it stores and compares the values of $this->content after each call.

+ * Tests the various "types" that a menu item may assume

Missing trailing dot. Also, this description, in general, makes me nervous.

...missing trailing dots all over the place.

+    $this->assertText('Menu Normal Type Test', t('The MENU_NORMAL_ITEM has been displayed on the page.'));

Please shorten the message, i.e. "MENU_NORMAL_ITEM is displayed." is sufficient.

+  /**
+   * Tests MENU_NORMAL_ITEM type
+   */
+  function testMenuNormalItemType() {
+    $this->drupalGet('node');
+    $this->assertText('Menu Normal Type Test', t('The MENU_NORMAL_ITEM has been displayed on the page.'));

There is no actual testing of the "menu path request result" here.

+  /**
+   * Tests MENU_CALLBACK type
+   */
+  function testMenuCallbackType() {
+    $this->drupalGet('menu-tests/types/menu_callback');
+    $this->assertResponse(200, t('MENU_CALLBACK loaded correctly.'));

Same here - a result of 200 can mean anything.

testMenuLocalTaskTypes() is the only valid test.

+    // Enable dummy module that implements hook_menu and callbacks.

No need for such a comment. It's obvious from the next line anyway.

+  $items['menu_callback_test/local_access_page_include_title'] = array(
+    'title' => 'Local Access & Page, Include Title',

I don't understand what "local" means in all these strings here. Cruft?

On that note, there does not seem to be a test that verifies the menu item's title is displayed.

+    'title arguments' => array(),
...
+    'access arguments' => array(),

Empty arrays don't need to be defined.

+function menu_callback_test_perm() {
+  return array(
+    'menu callback test access callback' => 'A permission for testing default menu callbacks.',
+  );
+}

This definition is actually wrong - I really wonder why the testbot does not complain.
http://api.drupal.org/api/function/page_example_perm/7 luckily contains a valid example.

+/**
+ * Local callback function for access permission
+ */
+function menu_callback_test_local_access() {
+  return TRUE;
+}

We should check for an actual permission - or even better, use some custom access check - here. TRUE means nothing, it's the default in most cases.

+  $items['menu_callback_test/access_denied'] = array(
+    'title' => 'Access Denied',
+    'access callback' => 'menu_callback_test_access_denied',
+    'access arguments' => array(),
+    'page callback' => 'menu_callback_test_local_page',
+  );

To properly test for access denied, the menu item should use a non-existent permission. I.e. no 'access callback', but according 'access arguments'. When using such code (i.e. a non-existing permission), a code comment should clarify what's done there.
Since tests are ran by uid 1 by default, the test needs to create and log in a separate user for this. (Something I miss in these tests anyway.) Bear in mind that uid 1 can access each and everything.

+++ modules/simpletest/tests/menu_callback_test.module	23 Apr 2009 15:45:28 -0000

Why do we introduce a separate .test and .module file for menu callback tests?

+  

Last, but not least, there is trailing white-space on lines that should be empty.

Wow, that is a little intense. I will look into the areas mentioned and make the improvements, but could you expand on a few points?

There is no actual testing of the "menu path request result" here.

Do you mean that I am not checking that when the link is followed it goes to the right page? I can see that the only thing being tested is that the link appears.

Same here - a result of 200 can mean anything.

In this case I guess it should check the content of the page, but 200 doesn't mean "anything". I was verifying that it didn't 404 while trying to reach the page.

I don't understand what "local" means in all these strings here. Cruft?

Local stands for a function that is within the file with hook_menu() in it. You may also notice that there are similarly named functions which use 'include' instead of 'local' as the functions being called are within the .inc file. I believe it helps provide a visual distinction between the locations of the functions. Looking at the comments I apparently made a copy/paste error but if you look at these functions you can see the test.

  /**
   * Test local access & page callbacks with included title callback
   */
  function testLocalAccessPageIncludeTitleCallback() {
    $this->drupalGet('menu_callback_test/local_access_page_include_title');
    $this->assertTitle('include title | Drupal', t('Included title found on page.'));
    $this->assertText('local content', t('Local text found on page.'));
  }
  
  /**
   * Test local access & page callbacks with included title callback
   */
  function testLocalPageTitleIncludeAccessCallback() {
    $this->drupalGet('menu_callback_test/local_page_title_include_access');
    $this->assertTitle('local title | Drupal', t('Local title found on page.'));
    $this->assertText('local content', t('Local text found on page.'));
  }
  
  /**
   * Test local access & page callbacks with included title callback
   */
  function testLocalAccessTitleIncludePageCallback() {
    $this->drupalGet('menu_callback_test/local_access_title_include_page');
    $this->assertTitle('local title | Drupal', t('Local title found on page.'));
    $this->assertText('include content', t('Included text found on page.'));
  }

This definition is actually wrong - I really wonder why the testbot does not complain.

I was looking at http://api.drupal.org/api/function/hook_perm/7 for proper syntax. If it needs to be in the format as specified by the page example module, I will update it.

We should check for an actual permission - or even better, use some custom access check - here. TRUE means nothing, it's the default in most cases.

The whole purpose here is to verify that the function gets called, should this be
return user_access('some perm');

To properly test for access denied, the menu item should use a non-existent permission. I.e. no 'access callback', but according 'access arguments'. When using such code (i.e. a non-existing permission), a code comment should clarify what's done there.
Since tests are ran by uid 1 by default, the test needs to create and log in a separate user for this. (Something I miss in these tests anyway.) Bear in mind that uid 1 can access each and everything.

In this instance we are returning FALSE, not even uid 1 can access the page when the access callback returns false (if it could the test wouldn't pass ;) ). What you are describing as a solution is test elsewhere, take a look at:

  /**
   * Test that the default access callback for menu items is working correctly.
   */
  function testMenuDefaultAccessCallback() {
    // Test with authorized user.
    $web_user = $this->drupalCreateUser(array('menu callback test access callback'));
    $this->drupalLogin($web_user);
    $this->drupalGet('menu_callback_test/default_access_callback');
    $this->assertResponse(200, t('User with permissions can access menu item using default access callback.'));

    // Test with an unauthorized user.
    $web_user = $this->drupalCreateUser();
    $this->drupalLogin($web_user);
    $this->drupalGet('menu_callback_test/default_access_callback');
    $this->assertResponse(403, t('User without permissions is denied access using default access callback.'));
  }

Why do we introduce a separate .test and .module file for menu callback tests?

I just thought they didn't fit with the other menu tests, thus I moved them into their own. All of these tests are very targeted to the menu callback subsystem. Looking at the menu test code it appears as though Drupal generates the proper trees and such. Should these tests be cobbled together with menu.module and menu.test?

Thanks for your help, I will fix a bunch of these and push out another patch.

Moving this out of the critical bugs queue - see #607038: Meta issue: fix gaps in code coverage.

Code sprint tag