TestingParty08: drupal_match_path()

This is brilliant, I was about to suggest the same thing. It seems that every module that wants to match an array of paths versus the current path needs to implement exactly the same code in their modules, which is a waste, especially since most of them just copy and paste it from the block.module, and modify slightly to suit their particular needs.

Big +1 from me. Don't know if it works, I don't have a 7.x-dev set up yet.

Very nice unit tests attached. :) Note, there is no current DrupalUnitTestCase, so we are forced to use DrupalWebTestCase for now.

This has 39 passes, 0 fails, and 0 exceptions on my machine.

I don't think it makes any sense to put all that code in the setUp() function. setUp() should be reserved for code that's shared between multiple tests. all of that is only used by the testDrupalMatchPath() test. i'd say move it into the test and if another case is added then move the shared code into setUp().

Rerolled as per drewish's comments. Moved the test cases set up into a separate function, called directly from the test function. Also added a set of test cases that makes sure regular expressions are properly escaped.

Updated patch to include tests in simpletest.info for the UI as well as modifying var syntax on one test for a clean test. Test now shows up and runs with the following result: 41 passes, 0 fails, and 0 exceptions

Fixing component and status.

Looks awesome! Only two problems:

- The function getInfo() should now be declared as both static and public.
- The function _drupalMatchPathTests() should lose the underscore and be declared as protected.

Thank you for the comments - much appreciated. Attached is the updated patch with the suggestions you recommended:
- function getInfo() declared as both static and public.
- function _drupalMatchPathTests() has no underscore and protected.
Patch tested with 41 passes, 0 fails, and 0 exceptions

Awesome.

Couple of very minor things I'm going to be a stickler on since this is the first time I've reviewed one of your patches and I want to impart best practices. :)

+++ modules/simpletest/tests/path.test	20 Sep 2008 17:29:13 -0000
@@ -0,0 +1,130 @@
+  /**
+   * Implementation of getInfo().
+   */
...
+  /**
+   * Implementation of setUp().
+   */

We don't actually document getInfo() or setUp() because they are inherited functions from the base class. Silly but true.

+++ modules/simpletest/tests/path.test	20 Sep 2008 17:29:13 -0000
@@ -0,0 +1,130 @@
+    return array(
+      'name' => t('Drupal match path'),
+      'description' => t('Tests the <a href="@url">drupal_match_path()</a> function to make sure it works properly.', array('@url' => 'http://api.drupal.org/api/function/drupal_match_path/7')),
+      // Use "Path API" rather than "Path" to distinguish it from path.module.
+      'group' => t('Path API'),
+    );
+  }

There should not be t()s around these strings, as there's never a reason for translators to translate them.

Other than that, the tests look great! If you do a quick re-roll we can get this sucker committed.

I'm on crack. Are you, too?

#293496 by cwgordon7, kathyh | dwees: Fixed TestingParty08: drupal_match_path().

Thank you for the comments - much appreciated. Per the suggestions:
-removed comments from inherited base classes - not needed
-removed t() from definitions - not used here

Also downloaded the greasemonkey script for this page, quite useful. thx.

Looks good.

changed status to needs review - have bot check one more time. Latest works on head - verify fail on bot.

OK - no changes to the code made, just re-ran the testbed and it passed again. Changing the status back to status prior to the re-check.

Marking as a quick fix because this requires no thought to commit. :)

Cool, did a couple other minor adjustments and committed to HEAD! Great work!