Tests for Shortcut module

I'll maintain this comment to track the sub-issues involved:

#614498: Add handling for query parameters in shortcut links
#680380: Shortcut bar cannot deal with deep menu items

I have been working on creating a .test file for the shortcut module and I'm making some progress, but there is a fundamental problem that I need some guidance on; namely, no matter what I do in terms of creating a user with the correct permissions and enabling the correct modules, the toolbar and the shortcut drawer are always empty when the tests are run (see the attached screenshot).

Many of the tests that are needed can actually still be tested, but fundamentally the shortcut links need to show up in order for us to consider them working...

Attached you'll find:
a) a screenshot showing the empty toolbar & shortcuts: http://img.skitch.com/20100120-denpsarkad697y25tn7tdxnk8p.jpg
b) a patch so that the tests will run (plus a tiny documentation fix)
c) shortcut.test file (goes in the root of /modules/shortcut)

Note: not all the shortcut tests work yet, because I need to solve the problem discussed above before I can proceed

Great work bleen18!

I just tried to put two additional permissions in your test for the admin user, 'administer comments' and 'access administration pages', which are the permissions needed to view the shortcuts that you are adding in do_testshortcuts() and I got the shortcuts bar filled nicely... See attached screenshot.

The only thing I changed in the test was this:

$this->admin_user = $this->drupalCreateUser(array('access toolbar', 'administer shortcuts', 'customize shortcut links', 'switch shortcut sets', 'administer comments', 'access administration pages'));

awesome!! ... back to making tests :)

Thanks fabsor!

I think I have this pretty close - definitely a good start.

I'm testing the following:

• create shortcuts for a variety of paths (I need more to check)
• checking that "add shortcut" changes to "remove shortcut" when needed
• create shortcut set
• shortcut set can be chosen as current set
• switch default shortcut set
• switch users shortcut set

I have the following "todo"s listed in the test:

1. //@todo: add more paths to check (including paths with query strings)
   any suggestions?
2. //@todo: check that you cant add shortcuts on stupid pages (ex. confirmation page when deleting a shortcut)
   I need someone to give me a better definition of "stupid pages" and any advice on how to check for this is welcome - I have no idea other than brute force checking of all stupid pages, but that's not practical. Do we even have a mechanism for this in place yet? In other words, is this (in theory) working yet?
3. //@todo: check that you cannot add a shortcut that already exists once that is possible
   this isnt possible yet, right?
4. //@todo: check removing shortcut works
   working on this test

Anything I'm missing? Any suggestions for my "todo's"?

1. What about adding some node/%nid/edit shortcuts ? We can create as many nodes as we want when testing, and we can be sure to have access to the page ?

2. As far as I know, there are no mechanism for stopping stupid pages yet, or if there are any plans to even have one. Anyone who knows?

I was working on a patch for a problem with shortcut.module here:

http://drupal.org/node/609122

As you can see in the comments, David_Rothstein commented that we need to make sure that only the shortcuts that are being saved with shortcut_set_save() should be in the set, and any shortcuts not in the links array passed to shortcut_set_save should be removed when calling shortcut_set_save(). We will need a test for that as well. The latest patch I contributed there does this, I think, but without a proper test for it, it's hard to know.

@fabsor: I was able to add a test for "node/%nid/edit" without a problem and I think I have a working test for the issue you pointed me to (#609122: shortcut_set_save() no longer deletes existing links when a new set of links is passed in)... I was a little unsure of whether mlids can change or not - I interpreted it as "no they cant". Take a look.

One more interesting thing to look at:
I added a bunch of nodes and then tried to test that a shortcut to "/node?page=1" was a valid shortcut, but I get an error that this is not a valid path. I think this might be a bug. Any thoughts?

You interpreted it right, mlids shouldn't be able to change. The test for checking that shortcut_set_save does the right thing seems right =)

Obviously this wont pass tests ... still needs review though

Neato ... webchick just showed me how to create a patch with new files included (http://wimleers.com/blog/cvs-diff-new-files-fakeadd). This patch includes the tiny change to include the tests, a quick fix to a comment in the shortcut module, AND the new shortcut.test file.

yay!! testbot has sobered up...

webchick said she was ready to commit this (on IRC) once testbot was back up ...

@webchick ... thanks for such a detailed review. I'm learning *a lot* about creating tests.

Lets see how close I got. A couple of notes though:

• I created an issue for shortcuts with query strings at #711448: Creating shortcuts to paths with query string... it's a bit of a mess

• The assertion should be checking for the same string as the Shortcut module defines, which is probably something like "Remove from %title shortcuts".

  Doing this was fine, but it then required an assertRaw instead of assertText

• Also, double-check what we do for naming conventions in other tests. I want to say we do drupalCreateX() but I'm not sure if that's consistent.

  There doesn't seem to be a convention. forum.test uses "generate", image.test uses "create", etc...

Nice work bleen18!

Quick review:

+++ modules/shortcut/shortcut.test	11 Feb 2010 15:30:18 -0000
@@ -0,0 +1,170 @@
+    // Reset changed variables.
+    variable_set('default_nodes_main', $old_default_nodes_main);

There is no need to save and revert this value, as each test case runs in its own environment, so you get a brand new Drupal for each testMethod.

+++ modules/shortcut/shortcut.test	11 Feb 2010 15:30:18 -0000
@@ -0,0 +1,170 @@
+    $this->assertTrue(count(array_intersect($old_mlids, $new_mlids)) == count($old_mlids)-1, 'shortcut_set_save() correctly updates existing links.');

Errr, huh? It seems to be checking whether or not an item was removed, although the message says something about updating... Probably a lack of sleep, but I think this could either use a comment or a simpler assertion...

+++ modules/shortcut/shortcut.test	11 Feb 2010 15:30:18 -0000
@@ -0,0 +1,170 @@
+  private function generateShortcutLink($path, $title=''){

Needs some spaces between the parameter name and the default value.

Powered by Dreditor.

Errr, huh? It seems to be checking whether or not an item was removed, although the message says something about updating... Probably a lack of sleep, but I think this could either use a comment or a simpler assertion...

The comment at the beginning of that section of the test is a little more detailed ... but I have also reworded the assertion comment to (hopefully) make it clearer. Basically, there have been problems with shortcut_set_save() resetting all the mlids for the entire set whenever it was called. This was deemed bad at [#6091222] and they requested a test be included ...

Just to some up (because people have asked me to in a couple other shortcut issues) the patch as it currently lives tests this:

• creating shortcut sets
• choosing shortcut sets
• admin assigning shortcut set to another user
• a variety of links can be used as shortcuts
• creating link
• the "add shortcut" link becomes "remove shortcut" when appropriate
• shortcut_set_save function properly updates shortcuts (without reassigning IDs) #609122: shortcut_set_save() no longer deletes existing links when a new set of links is passed in

Additional things I think should be tested:
- Deleting a shortcut set.
- Something reasonable happens when you try to delete the system default shortcut set (i.e. you can't and are warned).
- The hook that lets a contrib module define the default shortcut set for a user works -- hook_shortcut_default_set()
- Changing name of a shortcut set.
- Editing a shortcut link (name and/or path).
- Deleting a shortcut link from the edit shortcuts page (as opposed to the remove shortcut link).
- Adding a shortcut link from the edit shortcuts page
- What happens when you exceed the allowed number of shortcuts (latest added should be disabled)

I also took a look at the patch above. A few comments:

a)

+class ShortcutTestCase extends DrupalWebTestCase {
+  /**
+   * Defines shortcut test cases.
+   */

Docblock comments should appear *before* the declaration. So it should be:

+/**
+ * Defines shortcut test cases.
+ */
+class ShortcutTestCase extends DrupalWebTestCase {

b)

+  /**
+   * Test the creation, editing, deletion & usage of shortcuts.
+   */
+  function testShortcuts(){

Function and method docs should start with a verb in 3rd person, e.g. "Tests" not "Test".

c)

+      $settings = array(
+        'title' => $this->randomName(32),
+        'type' => 'article',
+      );
+      $node = $this->drupalCreateNode($settings);

This is inside a loop, and $node is overwritten each time? It's a WTF moment, because only the last one is kept? Also, you don't need to supply the node title. It will be randomized already in drupalCreateNode().

d) Testing style...
- Generally, setup stuff like creating nodes and assigning shortcut sets is done in the setup function.
- Generally, tests should be as small and atomic as possible.

f) No newline at end of file.

This patch is just me chasing HEAD ... I've also made the changes suggested in #20 but none of the extra tests suggested in #19 yet. Those will be coming shortly (aka tomorrow)

re #20:
the comments you made in (c) were remnants of an older version of the test that never got removed.

Generally, tests should be as small and atomic as possible

Are you suggesting there should be a testShortcutAdd() and a testShortcutDelete, and a testShortcutRename() function (and so on...) instead of just one testShortcut function?

This patch is just me chasing HEAD ... I've also made the changes suggested in #20 but none of the extra tests suggested in #19 yet. Those will be coming shortly (aka tomorrow)

re #20:
the comments you made in (c) were remnants of an older version of the test that never got removed.

Generally, tests should be as small and atomic as possible

Are you suggesting there should be a testShortcutAdd() and a testShortcutDelete, and a testShortcutRename() function (and so on...) instead of just one testShortcut function?

Not sure what that kind of fail means .. in the mean time,

- Something reasonable happens when you try to delete the system default shortcut set (i.e. you can't and are warned).

Currently you get an "Access denied" ... is that considered reasonable?

Ok ... I'm crossing out tests as I go:

- Deleting a shortcut set.
- Something reasonable happens when you try to delete the system default shortcut set (i.e. you can't and are warned). (Assuming "Access denied" == "reasonable")
- The hook that lets a contrib module define the default shortcut set for a user works -- hook_shortcut_default_set() Not sure how to test this ... any advice?
- Changing name of a shortcut set.
- Editing a shortcut link (name and/or path).
- Deleting a shortcut link from the edit shortcuts page (as opposed to the remove shortcut link).
- Adding a shortcut link from the edit shortcuts page
- What happens when you exceed the allowed number of shortcuts (latest added should be disabled)

- Access denied is OK on deleting the system shortcut.
- To test the hook, you need to make a small test module that implements the hook. Put this module into the "tests" subdirectory of module/shortcuts. Many other modules have examples of doing this in their tests.
- And yes, I am suggesting that each test be in its own function. When I made some tests for another issue recently, that was what I was told, so I am saving you time by telling you now so that while you are writing tests, you can make them into small, atomic test functions, rather than having to go back and redo the whole thing (at this point, you will only have to go back and redo a few).

Ok ... this patch has all the suggestions from #19 except the following:

- The hook that lets a contrib module define the default shortcut set for a user works -- hook_shortcut_default_set() This one make take m some time - it's new to me
- What happens when you exceed the allowed number of shortcuts (latest added should be disabled)This one I'll get to - just not sure if I can before next week

In addition I did not create separate functions for each test - if its considered important enough I'll come back to that later, but I think its more important to get some of these tests committed.

Sorry, I'm unexpectedly running a little low on time ...

Looks mostly reasonable. A few comments:

a) I'm not sure about whether the patch would be acceptable without breaking up the tests. Mine about two weeks ago wasn't, so I'm guessing not.

b)

+    $this->node = $this->drupalCreateNode(array('type' => 'article'));

This member variable node is not declared?

c) According to our coding standards, all comments in code should be sentences and start with capital letters; several of these don't.

d)

+    $new_link_name = $this->randomName(10);
+    $new_link_path = 'admin/help';

I'm confused as to why this is working, since you didn't put "help" as one of the modules for this test. Probably should be added to setup, or a different path should be used for this test. Also, above you are using a comment module path -- again, this module should be added to setup. I see that the tests pass, but I'm not convinced they should have worked? Neither of those is a generally required core module?

e) Your shortcut add/delete tests don't actually verify that the shortcuts can be viewed in the shortcut set, or that after deletion they aren't visible any more, or that if you load the shortcut set using shortcut_set_load(), the shortcut is or is not part of the array.

f)

+    // Attempt to switch the default shortcut set to the newly created shortcut set.
+    $this->drupalPost(NULL, array('set' => 'shortcut-set-2'), t('Change set'));

This should probably use $set->set_name (which I think/hope should be filled in -- it's used later on anyway), rather than assuming it's going to be 'shortcut-set-2'. Same for later in the same function.

g)

+    $set->links[] = $this->generateShortcutLink('admin', $this->randomName(10));
+    shortcut_set_save($set);
+
+    foreach ($set->links as $link){
+      $new_mlids[] = $link['mlid'];
+    }
+
+    $this->assertFalse(in_array($deleted_mlid, $new_mlids), 'shortcut_set_save() correctly deletes existing links.');
+    $this->assertTrue(count(array_intersect($old_mlids, $new_mlids)) == count($old_mlids)-1, 'shortcut_set_save() did not inadvertently change existing mlids.');
+

This test needs to load the saved shortcut set after saving. As it is now, you are just testing the array you created, not the array you would get if the shortcut set were loaded, because shortcut_set_save() is not loading it, as far as I can tell? Anyway, it would definitely be a more convincing test to me if it did a shortcut_set_load().

h)

+    // Attempt to rename a shortcut set.
+    $new_title = $this->randomName(10);
+    $this->drupalPost('admin/config/user-interface/shortcut/'. $set->set_name .'/edit', array('title' => $new_title), t('Save'));
+    $set = shortcut_set_load($set->set_name);
+    $this->assertRaw(t('Updated set name to %title.', array('%title' => $set->title)));

You didn't actually test $set to verify it has the right set title, just looked at the message on the screen.

i)

+    // Attempt to delete a shortcut set.
+    $this->drupalPost('admin/config/user-interface/shortcut/'. $set->set_name .'/delete', array(), t('Delete'));
+    $this->assertRaw(t('The shortcut set %title has been deleted.', array('%title' => $set->title)), 'Successfully deleted a shortcut set.');

You didn't actually verify that the set no longer exists, just that the correct message was generated.

@bleen18: Thanks for working on this - this is great!

My suggestion would be that for this first patch, there's no need to test any more functionality than what you've already done - probably best at this point to just focus on getting these existing tests in good shape (rather than adding the missing ones you noted in #27). I doubt there's any module in core that has anything close to 100% test coverage, so the goal here doesn't need to be to test everything, just to get a basic set of tests in place... since right now, the shortcut module, uh, doesn't have any :) After this is committed, there can be followup issues for adding more tests - which will be easier for people to work on once the basic framework here is in place.

My only general comment along those lines is that all these tests are currently run as a user with 'administer shortcuts' permissions. We might be able to get some "free" extra test coverage by having a user with more limited permissions ('customize shortcut links' or 'switch shortcut sets') run some of them instead, where appropriate.

I also definitely agree that these should be broken up, although I don't know if it's a requirement for commit or not (I think there are plenty of existing mega-test-cases in core right now dating back from the early days of SimpleTest, but those should be fixed too, of course). Either way, it's a good idea. When doing this, I think I'd recommend just turning the patch's existing test functions into classes, since those are good for holding large chunks of functionality. So, something like this:

class ShortcutTestCase extends DrupalWebTestCase {

  function setUp() {}

  /**
   * Helper functions go here.
   */
  protected function generateShortcutSet() {}
  protected function generateShortcutLink() {}

}

/**
 * All the stuff currently in testShortcuts() goes in here.
 */
class ShortcutLinksTestCase extends ShortcutTestCase {

  functon setUp() {}

  function testShortcutLinkAdd() {}
  function testShortcutLinkDelete {}

}

/**
 * All the stuff currently in testShortcutSets() goes in here.
 */
class ShortcutSetTestCase extends ShortcutTestCase {

  functon setUp() {}

  function testShortcutSetAdd() {}
  function testShortcutSetDelete {}

}

I'm confused as to why this is working, since you didn't put "help" as one of the modules for this test. Probably should be added to setup, or a different path should be used for this test. Also, above you are using a comment module path -- again, this module should be added to setup. I see that the tests pass, but I'm not convinced they should have worked? Neither of those is a generally required core module?

I think these work because both of those modules are enabled in the standard installation profile, which simpletest is hardcoded to use when running tests. I'm not sure if it's considered good practice to explicitly add them to setUp() anyway, though. It does seem like it would be a good idea, because it would make the code a bit more readable and future-proof.

Also, if you don't feel like you have the time to update the patch, comment here (and unassign yourself), and perhaps someone else will take it over.

It's a great start, close to being ready to commit, and I agree with David that not everything has to be tested in the first pass.

I'll play more this weekend ... thanks for all the reviews :)

I feel good about this one...

A couple of notes:

• re #28 (g): You were absolutely correct. When I rejiggered that test, I found that shortcut_set_save() does not handle deleted shortcuts; only newly added ones. I'm not sure what is intended there so I commented out a small chunk of code in the test in case its useful once I get a bit of clarification on what *should* be tested there
• re #29: followed your advice on how to structure the file. Works fine, but tests are much much much slower now :(
• re #28 (d): Just to avoid confusion I changed to use some generic system paths
• re #28 (h) & (i): DOH!! - these are fixed

Agreed: Splitting up the tests does make them slower. However, it also makes it easier to pinpoint which part of the test is failing if some proposed patch makes tests fail. So... we live with the slowdown.

Is there an issue filed on the bug that shortcut_set_save() doesn't get rid of links that shouldn't be there any more?

I took a look at your new patch. It's getting closer... A few suggestions:

a) Due to #28 (g), how about loading the set right in this function, adding an assert that it exists, and returning the loaded set rather than the object you create inside the function?

+  function generateShortcutSet($title = '', $default_links = TRUE) {
+    $set = new stdClass;
+    $set->title = empty($title) ? $this->randomName(10) : $title;
+    if ($default_links){
+      $set->links = array();
+      $set->links[] = $this->generateShortcutLink('node/add');
+      $set->links[] = $this->generateShortcutLink('admin/content');
+    }
+    shortcut_set_save($set);
+
+    return $set;
+  }

b) Typo:

+  /**
+   * Return an array of mlids withint the given set.
+   */

"withint" should be "within". Also, can we avoid "mlids" and instead write out "menu link IDs"?

Another typo in a comment:

+  //@todo: check that you cant add shortcuts on stupid pages (ex. confirmation page when deleting a shortcut).

c) Generally, all function doc headers need to start with a 3rd-person verb, such as "Creates" rather than "Create.
http://drupal.org/node/1354 has the doxygen standards, for reference.

d) I don't think your base class for shortcut testing should have getInfo() in it, as it has no tests.

e) Your class member variables should each have a short doc header. E.g.:

  /**
    * User with permission to administer shortcuts.
    */
  protected $admin_user;

f) Your classes should each have doc headers also.

g) testShortcutLinkAdd() still doesn't check that the link exists in the shortcut set, that I can see, but only that the response message is correct? Same applies to testShortcutLinkRename() -- not checking that the name has changed. And testShortcutLinkChangePath().

h)

/**
+   * Check that the "add to shortcut" link changes to
+   * "remove shortcut" when appropriate.
+   */

Doc headers need to start with a 1-line summary of 80 characters or less. See guidelines page link above. This applies to several other doc headers as well.

i) The member variable $set should be declared in the base test class, as it is defined in the base class's setup function.

I mistakenly introduced that bug with the shortcuts not being deleted in shortcut_set_save over here: http://drupal.org/node/609122
There's a patch that supposedly fixes the problem in there as well.

this patch has everything from #35 except (a)... I guess I dont see why your suggestion would work any better/worse than whats already there?

Thoughts?

this patch is the same as the one in #37 except one of the helper functions is slightly more efficient now...

DOH! I think I was still asleep when I posted the patch in #38 and inserted my special brand of stupidity.

Quick review... Sorry to be so picky, but unfortunately for you, you got a doc and standards obsessive person to review your patch. :)

This is very close, and I only saw a few issues:

a) You didn't address #35 (c) (or at least not completely:

+  /**
+   * Create a generic shortcut set.
+   */
+  function generateShortcutSet($title = '', $default_links = TRUE) {

This needs to be "Creates" not "Create". One or two other spots like this are remaining in the code.

b)

+  /**
+   * Searches the given shortcut set for variaous information.
+   * 
+   * @param object $set
+   *   The shortcut set to cull for information.
+   *
+   * @param string $key
+   *   The $link array key indicating what information to gather.
+   *
+   * @return
+   *   array of information.
+   */

This helper function doc needs a rewrite. You are not "culling" information (cull = delete sick members from a herd, not extract and return information), the doc doesn't really tell me what the function does, we don't leave blank lines between @param declarations, and the @return should start with a capital letter and have more specifics. "various" is also misspelled in the first line.

Here's my suggestion, in case you aren't fond of writing documentation for functions (you may need to wrap this at 80 character lines, indent, etc.):

/**
  * Extracts information from shortcut set links.
  * 
  * @param object $set
  *   The shortcut set object to extract information from.
  * @param string $key
  *   The array key indicating what information to extract from each link:
  *    - 'link_path': Extract link paths.
  *    - 'link_title': Extract link titles.
  *    - 'mlid': Extract the menu link item ID numbers.
  *
  * @return array
  *   Array of the requested information from each link.
  */

c) in testShortcutLinkAdd() -- what is this?

isset($test['options']) ? $test['options'] : array());

Doesn't look like it's being used.

d) Also in that same function, I'm not sure why you took out the test that the message "shortcut created" was there? It seemed like a good idea, in addition to the new test that the link is actually in the saved shortcut (which looks good, by the way). Same comment applies to the other tests that you added to.

e) In testShortcutLinkDelete(), I think you can use your helper function here?

+    $mlids = array();
+    foreach($saved_set->links as $link) {
+      $mlids[] = $link['mlid'];
+    }

f) in testShorcutSetAssign():

+    shortcut_set_assign_user($new_set, $this->shortcut_user);
+    $this->drupalPost('user/'. $this->shortcut_user->uid .'/shortcuts', array('set' => $new_set->set_name), t('Change set'));

Don't these two lines do the same thing in two different ways? Probably should be two separate tests. I'm also unsure why you had to login as the non-priveleged user on the next line.

g) Spelling typo: function testShortcutSetSwtichOwn() should be SwitchOwn

Incidentally, cull also means to "collect or gather" ... but thats ok :)

@jhodgdon: "Sorry to be so picky, but unfortunately for you, you got a doc and standards obsessive person to review your patch. :)"

This is a good thing ... I'm still learning a lot of this.

Anywho... I can see the light at the end of the tunnel now.

This looks great! Two extremely minor things:

+  /**
+   * Shortcut set with two links.
+   */
+  protected $set;

... in setUp():

+    $this->set = shortcut_set_load('shortcut-set-1');

It looks like now $this->set is pointing to the site default shortcut set - which does have two links after installation, but maybe that code comment is not too precise?

Then one of the subclasses does:

+class ShortcutSetsTestCase extends ShortcutTestCase {
+
+  protected $set;

Probably no need to redefine $set here?

These were so minor, I just rerolled, and would advocate for RTBC assuming the test bot agrees.

As a reminder, if this gets committed, we need to set back to needs work so we can write more tests, or else file a follow-up issue (maybe that would be better).

I dont think I'm the one who should mark this RTBC ... but this patch worked fine for me

OK, I'll mark it RTBC. :)

I think this is almost RTBC, but not quite. Great job, though! I learned some things I didn't know about the testing framework by reading this (e.g., assertLinkByHref - had no idea about that one).

If you fix these things, then I think it's RTBC. (There are a bunch, but almost all are pretty simple to fix.)

1. +  public static function getInfo() {
   +    return array(
   +      'name' => 'Shortcut functionality',
   +      'description' => 'Create, view, edit, delete, and change shortcuts and shortcut sets and verify their functionality.',
   +      'group' => 'Shortcut',
   +    );
   +  }
   

   One of the reviews above mentioned this should be taken out, but it's still here :) We need to remove it because otherwise it shows up in the UI as a test case that can be run, and then when you try to run it you get an error message that there are no tests for it.

2. +    parent::setUp('menu', 'toolbar', 'shortcut');
   +    // Create users.
   +    $this->admin_user = $this->drupalCreateUser(array('access toolbar', 'administer shortcuts', 'customize shortcut links', 'switch shortcut sets', 'administer site configuration', 'access content', 'create article content', 'create page content', 'edit any article content'));
   +    $this->shortcut_user = $this->drupalCreateUser(array('access toolbar', 'customize shortcut links', 'switch shortcut sets', 'administer site configuration', 'access content', 'administer nodes', 'edit any article content'));
   

   These are an awful lot of modules and permissions, and some are definitely unnecessary. For example, if the admin user has 'administer shortcuts', they shouldn't need the two other shortcut module permissions also. I believe I was able to get all the tests passing by reducing the list to look like this:

       parent::setUp('toolbar', 'shortcut');
       // Create users.
       $this->admin_user = $this->drupalCreateUser(array('access toolbar', 'administer shortcuts', 'create article content', 'create page content', 'access content overview'));
       $this->shortcut_user = $this->drupalCreateUser(array('customize shortcut links', 'switch shortcut sets'));
   

   Let's try to get the minimum number of modules and permissions enabled here so it's cleaner, and if necessary, explain in a code comment why some of the odd ones are needed (I guess we need some so as to give the user enough permissions to visit the page in question and/or see a link to it in the shortcut bar?)

3. +      $this->assertLink($title, 0, 'Shortcut created: '. $test['path']);
   

   The use of assertLink() - here and elsewhere - I think we get false positives from it? The same link might display (and in fact does display) elsewhere on the page, e.g. in another menu, so the test will pass even if the shortcut never gets displayed as intended.

   What I think we ideally want to do here is assert that it appears within the shortcut area of the toolbar. This stuff gets a bit complicated because it involves parsing the HTML, but from http://www.w3schools.com/XPath/xpath_syntax.asp and elsewhere I think we want something like this:

   // This should give an array of all links within the "toolbar-shortcuts" div.
   $links = $this->xpath("//div[@class='toolbar-shortcuts']//a");
   

   You can then check that array to see if the desired link is in it.

   It's worth a shot, but I might have it wrong, so if it gets too complicated, let's not block this issue over it - we could save it for a followup.

4. +    $this->set = shortcut_set_load('shortcut-set-1');
   

   Here (and in other places in the patch), we should use the defined constant SHORTCUT_DEFAULT_SET_NAME instead of 'shortcut-set-1'. Basically, you can just do a find-and-replace on the patch file to fix those.....

5. +    if ($default_links){
   

   There should be a space between ")" and "{".

6. +  function setUp(){
   +    parent::setUp();
   +  }
   

   You have this in both ShortcutLinksTestCase and ShortcutSetsTestCase but in neither case is it necessary. You only need to add these if there is something specific that needs to be done; the parent one gets called automatically if the child class doesn't define a setup function. So, in both cases this code is redundant, and the function can just be removed.

7. +    // @todo: add more paths to check.
   +    $test_cases = array(
   +      array('path' => 'admin'),
   +      array('path' => 'admin/config/system/site-information'),
   +      array('path' => "node/{$this->node->nid}/edit"),
   +      // @todo: shortcuts to paths with query strings do not get added properly yet.
   +      // see http://drupal.org/node/711448.
   +    );
   

   I don't think we should have specific @todos like this here, or at least not so many. There are a million things in Drupal that could be tested but aren't, and we don't want to document them all :) So let's at least remove the first one.

   For the second, I could go either way - but I think it might be better to remove it from the patch and instead just add a comment to the relevant drupal.org issue explaining the test that should be added there.... I guess in general, my opinion is that @todos should only be in the code when we think that someone who comes along and reads the code would be obviously inclined to think "gee, I really can't figure out why XYZ wasn't done here". I'm not so sure that's the case here.

8. +    // Check that once each test case is added as a shortcut,
   +    // that the shortcut links where it should.
   +    foreach ($test_cases as $test){
   +      $title = $this->randomName(10);
   +      $form_data = array(
   +        'shortcut_link[link_title]' => $title,
   +        'shortcut_link[link_path]'  => $test['path']
   +      );
   +      $this->drupalPost('admin/config/user-interface/shortcut/'. $set->set_name .'/add-link', $form_data, t('Save'));
   

   A number of minor things here:

   • Maybe just me, but I think the code comment has one too many "that"s in it - I stumbled over it while reading it. I think we want: "Check that once each test case is added as a shortcut, the shortcut links to where it should."
   • The code comment wraps too early (it should go up to around 80 characters, without exceeding that).
   • The foreach statement is missing a space between ")" and "{".
   • Best coding style practices is to put a comma after all array elements, even the last one - i.e., there should be a comma at the end of 'shortcut_link[link_path]' => $test['path'].
   • D7 coding standards are to have a space both before and after the "." when concatenating strings, but the $this->drupalPost() line above doesn't do that. (Note: There are bunch of other places in the patch with the same problem; this is just the first one I noticed, so please fix all the ones you can find - thanks!)

9. +      $this->assertTrue(in_array($test['path'], $paths), 'Shortcut created: '. $test['path']);
   +      $this->assertLink($title, 0, 'Shortcut created: '. $test['path']);
   

   Shouldn't we use a different assertion message for the second test than the first - maybe something like "Shortcut link found on the page: ..."?

10. +  //@todo: check that you can't add shortcuts on stupid pages (ex. conformation page when deleting a shortcut).
    +  //@todo: check that you cannot add a shortcut that already exists once that is possible.
    

    As per above, let's remove these as well, and make separate issues for them, or move them to the existing issue if appropriate - e.g., a test for the first one would be added as part of any patch that happens at #635814: Some pages should not display the "Add to shortcuts" link.

11. +    return array(
    +      'name' => 'Shortcut Set functionality',
    

    "Set" should be lower-case here.

12. +  /**
    +   * Tests creating a shortcut set.
    +   */
    +  function testSortcutSetAdd() {
    

    Typo in the function name ("Sortcut").

13. +    $this->assertText($new_set->title, 'Generated shortcut set was listed as choice.');
    

    Should at least be "as a choice"... but how about something more like "Generated shortcut set was listed as a choice on the user account page."?

14. +    $this->assertTrue($new_set->set_name == $current_set->set_name, 'Successfully switched default shortcut set.');
    

    "default" is confusing here - I think this should be "Successfully switched own shortcut set", especially by analogy with the next test case where another user's set is being switched.

15. +  /**
    +   * Tests that shortcut_set_save correctly updates existing links.
    +   */
    

    Function names should be written as "..shortcut_set_save().."

16. +    /* 
    +    @todo: Currently shortcut_set_save() does not handle deleted links
    +           so the code below fails.
    +    $link = array_pop($set->links);
    +    $deleted_mlid = $link['mlid'];
    +    shortcut_set_save($set);
    +    $saved_set = shortcut_set_load($set->set_name);
    +    */
    

    Same as above - let's remove this @todo from the patch and add it as part of the followup at #609122: shortcut_set_save() no longer deletes existing links when a new set of links is passed in (for which I badly owe a review, but want to get this one committed first so we have a framework in place to write a test for that issue over there).

17. +  /**
    +   * Tests deleting the defailt shortcut set.
    +   */
    

    Typo here ("defailt").

on it

re #47

I've been learning a lot about the testing framework (and coding standards) too.... :)

(1) Must have missed it
(2) I only put in so many because of #2 & #4 in this issue ... but everything seems to work with your suggestion
(3) I couldn't quite get your suggestion to work .. but it occurs to me that the only places that these links might show up are places that would indicate that the shortcut was correctly created... anyway, like you said - this shouldn't hold anything up
(4) done
(5) done
(6) I was wondering about that...done
(7) wasnt sure about how to handle todo's
(8) done
(9) done
(10) see #7 - done
(11) done
(12) fat thumbs
(13) done
(14) done
(15) done
(16) see #7 - done
(17) fat thumbs

Wonderful!

In the attached, I removed 'menu' from the list of enabled modules (since the menu UI is definitely not used anywhere in these tests) and made two or three other minor fixes that were too small to bother sending this back for another round :)

Great job with this - thanks. Definitely RTBC as far as I'm concerned.

Great! Hopefully this will go in soon.

Now who wants to start the issue about "more tests are needed for the shortcut module", by combing through the comments above and figuring out what hasn't been tested yet?

#733924: Investigate where Shortcut module lacks test coverage

Committed to CVS HEAD. Thanks.