Clean up API docs for system module tests, A-D, including subdirectories

Here is my patch for just actions.test
My first patch :D
More to come!

Ok my second try at the patch i think i understand now!
If not let me know and I will try, try again.

Ok another patch for another file!

IGNORE PREVIOUS PATCHES THIS IS THE FINAL.
This is the final patch that I'm doing for this I've done a 3651 line patch file.
NO MORE!!!
Jk I'm a beginner and I've done what I can.

This is a big patch, but I read through all of it, nice work.

I changed a small handful of things and rolled a new patch. I think this is good, and the only changes are in comments.

--Rob

Just a quick review of two common mistakes to keep you moving! I realize you did not introduce these things, but since you are changing the stuff anyway, fixing it would be an ideal scenario.

+++ b/core/modules/simpletest/tests/actions.testundefined
@@ -10,7 +18,7 @@ class ActionsConfigurationTestCase extends DrupalWebTestCase {
   /**
-   * Test the configuration of advanced actions through the administration
+   * Tests the configuration of advanced actions through the administration
    * interface.
    */
   function testActionConfiguration() {

+++ b/core/modules/simpletest/tests/actions_loop_test.moduleundefined
@@ -52,8 +57,9 @@ function actions_loop_test_log() {
 /**
- * Replacement of the watchdog() function that eliminates the use of semaphores
- * so that we can test the abortion of an action loop.
+ * Replaces the watchdog() function so that we can test the abortion of an
+ * action loop.  This function is like the watchdog() function except that it
+ * does not use semaphores.
  */
 function watchdog_skip_semaphore($type, $message, $variables = array(), $severity = WATCHDOG_NOTICE, $link = NULL) {

Intro comments on functions/methods should be on a single line. If you modify an existing one, "unfortunately" it would be ideally coupled with fixing the multiline-ness of it too.

Good twitter skills pay off big time here.

There might be other instances of this problem, I just found these at the top.

+++ b/core/modules/simpletest/tests/ajax.testundefined
@@ -10,7 +18,7 @@ class AJAXTestCase extends DrupalWebTestCase {
   /**
-   * Assert that a command with the required properties exists within the array of Ajax commands returned by the server.
+   * Asserts that a command with the required properties exists within the array of Ajax commands returned by the server.

@@ -67,7 +75,7 @@ class AJAXFrameworkTestCase extends AJAXTestCase {
   /**
-   * Test that ajax_render() returns JavaScript settings generated during the page request.
+   * Tests that ajax_render() returns JavaScript settings generated during the page request.

@@ -116,7 +124,7 @@ class AJAXFrameworkTestCase extends AJAXTestCase {
   /**
-   * Test that new JavaScript and CSS files added during an AJAX request are returned.
+   * Tests that new JavaScript and CSS files added during an AJAX request are returned.

These and possibly others are above 80 chars. That is another typical problem (which is an attempt to avoid the multilines), but should not be used.

This is even tougher than twitter, you have much less space here...

I went through the patch one more time and fixed the common errors from jhodgdon and Gàbor. This patch should fix the 2-line issues as well as all the 80-column issues from the previous patches.

--forgot to change the status of the issue.

Thanks @ChinggizKhan!

Updating for #1299424: Allow one module per directory and move system tests to core/modules/system. This patch will probably need a rebase.

#13: simpletest-cleanup_api_a_to_d-1431658-13.patch queued for re-testing.

Re-rolled. Attached is a giant interdiff as well.

I think your interdiff is backwards. :) Edit: To be clear, that's just an observation--don't worry about making another in this case. Starting a review on this now.

Thanks @connork, @rgristroph, @ChinggizKhan, and @kid_icarus for all your work on this patch. It's a big one!

I reviewed about 20% of the patch, starting from the bottom up, but stopped there because there are just too many outstanding issues. (It's already taken me an hour reviewing that much.) Couple general observations:

• I'm guessing maybe the patch here used a script to append an S to the first word of docblocks if it didn't have one? That's actually a fine strategy as a first pass, but please review the changes before submitting the patch to make sure they're valid.
• Many of the updated docblocks are too long. They should be a single line of 80 characters or fewer. Additional information can be placed in subsequent paragraphs. Reference: http://drupal.org/node/1354#functions

Specifics from as much as I got through:

1. +++ b/core/modules/system/tests/bootstrap.testundefined
   @@ -389,8 +409,8 @@ class BootstrapTimerTestCase extends DrupalUnitTestCase {
   -   * Test timer_read() to ensure it properly accumulates time when the timer
   -   * started and stopped multiple times.
   +   * Tests timer_read() to ensure it properly accumulates time when the timer
   +   * is started and stopped multiple times.
   
   @@ -424,7 +444,7 @@ class BootstrapResettableStaticTestCase extends DrupalUnitTestCase {
   -   * Test that a variable reference returned by drupal_static() gets reset when
   +   * Tests that a variable reference returned by drupal_static() gets reset when
       * drupal_static_reset() is called.
   
   +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
   @@ -756,7 +756,7 @@ class DatabaseUpdateTestCase extends DatabaseTestCase {
   -   * Confirm that we can update a multiple records with a non-equality condition.
   +   * Confirms that we can update a multiple records with a non-equality condition.
   
   @@ -1134,7 +1134,7 @@ class DatabaseMergeTestCase extends DatabaseTestCase {
   -   * Confirm that we can merge-update a record successfully, with different insert and update.
   +   * Confirms that we can merge-update a record successfully, with different insert and update.
   
   @@ -1155,7 +1155,7 @@ class DatabaseMergeTestCase extends DatabaseTestCase {
   -   * Confirm that we can merge-update a record successfully, with alternate replacement.
   +   * Confirms that we can merge-update a record successfully, with alternate replacement.
   
   @@ -1261,7 +1261,7 @@ class DatabaseMergeTestCase extends DatabaseTestCase {
   -   * Test that an invalid merge query throws an exception like it is supposed to.
   +   * Tests that an invalid merge query throws an exception like it is supposed to.
   
   @@ -1508,7 +1508,7 @@ class DatabaseSelectTestCase extends DatabaseTestCase {
   -   * Test that we can UNION multiple Select queries together. This is
   +   * Tests that we can UNION multiple Select queries together. This is
       * semantically equal to UNION DISTINCT, so we don't explicity test that.
   
   @@ -2096,7 +2096,7 @@ class DatabaseSelectComplexTestCase extends DatabaseTestCase {
   -   * Test that countQuery properly removes 'all_fields' statements and
   +   * Tests that countQuery properly removes 'all_fields' statements and
       * ordering clauses.
   
   @@ -2464,8 +2469,8 @@ class DatabaseSelectTableSortDefaultTestCase extends DatabaseTestCase {
   -   * Confirm that if a tablesort's orderByHeader is called before another orderBy, that the header happens first.
   -   *
   +   * Confirms that if a tablesort's orderByHeader is called before another
   +   * orderBy, that the header happens first.
   
   @@ -2490,7 +2495,8 @@ class DatabaseSelectTableSortDefaultTestCase extends DatabaseTestCase {
   -   * Confirm that if a sort is not set in a tableselect form there is no error thrown when using the default.
   +   * Confirms that if a sort is not set in a tableselect form there is no error
   +   * thrown when using the default.
   
   @@ -2700,7 +2706,7 @@ class DatabaseAlterTestCase extends DatabaseTestCase {
   -   * Test that we can remove a range() value from a query. This also tests hook_query_TAG_alter().
   +   * Tests that we can remove a range() value from a query. This also tests hook_query_TAG_alter().
   
   @@ -2853,7 +2859,7 @@ class DatabaseLoggingTestCase extends DatabaseTestCase {
   -   * Test that we can log queries against multiple targets on the same connection.
   +   * Tests that we can log queries against multiple targets on the same connection.
   
   @@ -2875,7 +2881,7 @@ class DatabaseLoggingTestCase extends DatabaseTestCase {
   -   * Test that logs to separate targets collapse to the same connection properly.
   +   * Tests that logs to separate targets collapse to the same connection properly.
   
   @@ -3179,7 +3185,8 @@ class DatabaseInvalidDataTestCase extends DatabaseTestCase {
   -   * Traditional SQL database systems abort inserts when invalid data is encountered.
   +   * Traditional SQL database systems abort inserts when invalid data is
   +   * encountered.
   

   All these summaries need to be one line of fewer than 80 characters, beginning with a third-person verb. If there's additional information to add, we can add it in a subsequent paragraph (as in the case of several here include a second sentence).

2. +++ b/core/modules/system/tests/bootstrap.testundefined
   @@ -389,8 +409,8 @@ class BootstrapTimerTestCase extends DrupalUnitTestCase {
       * @return
       */
      function testTimer() {
   

   The @return here should be removed. Test methods don't return anything.

3. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
   @@ -2346,7 +2349,7 @@ class DatabaseSelectPagerDefaultTestCase extends DatabaseTestCase {
   -   * Confirm that a pager query with inner pager query returns valid results.
   +   * Confirms that a pager query with inner pager query returns valid results.
   

   Can we fit "an" before "inner pager query" here, or otherwise reword this? It's a little confusing.

4. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
   @@ -2367,7 +2370,7 @@ class DatabaseSelectPagerDefaultTestCase extends DatabaseTestCase {
   -   * Confirm that a paging query with a having expression returns valid results.
   +   * Confirms that a paging query with a having expression returns valid results.
   
   @@ -3422,7 +3429,7 @@ class DatabaseTransactionTestCase extends DatabaseTestCase {
   -   * Test transaction rollback on a database that does not support transactions.
   +   * Tests transaction rollback on a database that does not support transactions.
   

   The correction for the verb forms here puts these line over 80 characters, so we need to reword them to be shorter.

5. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
   @@ -2387,7 +2390,7 @@ class DatabaseSelectPagerDefaultTestCase extends DatabaseTestCase {
   -   * Confirm that every pager gets a valid non-overlaping element ID.
   +   * Confirms that every pager gets a valid non-overlaping element ID.
   

   Let's add a comma after "valid" here.

6. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
   @@ -2500,7 +2506,7 @@ class DatabaseSelectTableSortDefaultTestCase extends DatabaseTestCase {
   - * Select tagging tests.
   + * Selects tagging tests.
   
   @@ -2584,7 +2590,7 @@ class DatabaseTaggingTestCase extends DatabaseTestCase {
   - * Select alter tests.
   + * Selects alter tests.
   

   These are not selecting anything. They are testing select query alters and tagging.

7. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
   @@ -2516,7 +2522,7 @@ class DatabaseTaggingTestCase extends DatabaseTestCase {
   -   * Confirm that a query has a "tag" added to it.
   +   * Confirms that a query has a "tag" added to it.
   

   I don't think "tag" needs to be in quotes here.

8. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
   @@ -2559,7 +2565,7 @@ class DatabaseTaggingTestCase extends DatabaseTestCase {
   -   * Test that we can attach meta data to a query object.
   +   * Tests that we can attach meta data to a query object.
   

   "Metadata" should be one word.

9. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
   @@ -2700,7 +2706,7 @@ class DatabaseAlterTestCase extends DatabaseTestCase {
   -   * Test that we can remove a range() value from a query. This also tests hook_query_TAG_alter().
   +   * Tests that we can remove a range() value from a query. This also tests hook_query_TAG_alter().
   
   @@ -2853,7 +2859,7 @@ class DatabaseLoggingTestCase extends DatabaseTestCase {
   -   * Test that we can log queries against multiple targets on the same connection.
   +   * Tests that we can log queries against multiple targets on the same connection.
   
   @@ -2875,7 +2881,7 @@ class DatabaseLoggingTestCase extends DatabaseTestCase {
   -   * Test that logs to separate targets collapse to the same connection properly.
   +   * Tests that logs to separate targets collapse to the same connection properly.
   
   @@ -3248,7 +3255,7 @@ class DatabaseQueryTestCase extends DatabaseTestCase {
   -   * Test that we can specify an array of values in the query by simply passing in an array.
   +   * Tests that we can specify an array of values in the query by simply passing in an array.
   

   These needs to be reworded to be under 80 characters.

10. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
    @@ -2741,7 +2747,7 @@ class DatabaseAlterTestCase extends DatabaseTestCase {
    - * Regression tests.
    + * Tests regressions.
    

    I don't think this is testing regressions. I think it's testing to make sure that there aren't regressions... However, I think the naming of the class itself is kind of questionable, so I'm not sure what to change this to. I might look at what the setUp() method does and what all the methods are, and try to draw a picture from that.

11. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
    @@ -2758,7 +2764,7 @@ class DatabaseRegressionTestCase extends DatabaseTestCase {
    -   * Regression test for #310447.
    +   * Tests regression for #310447.
        *
        * Tries to insert non-ascii UTF-8 data in a database column and checks
        * if its stored properly.
    

    The rewording here is inaccurate. However, this docblock is bad to begin with, because the summary should explain what the function does, not reference something somewhere on the internet. How about replacing both these sentences with:
    "Ensures that non-ASCII UTF-8 data is stored in the database properly."

12. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
    @@ -2929,7 +2935,7 @@ class DatabaseLoggingTestCase extends DatabaseTestCase {
    - * Query serialization tests.
    + * Querys serialization tests.
    

    Aside from the fact that "queries" is spelled incorrectly, the function isn't querying serialization tests, it's testing query serialization. So let's change this to:
    "Tests query serialization."

13. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
    @@ -2956,7 +2962,7 @@ class DatabaseSerializeQueryTestCase extends DatabaseTestCase {
    - * Range query tests.
    + * Ranges query tests.
    

    I am pretty sure we are not ranging anything here. :) Let's make this "Tests range queries."

14. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
    @@ -2972,7 +2978,7 @@ class DatabaseRangeQueryTestCase extends DrupalWebTestCase {
    -   * Confirm that range query work and return correct result.
    +   * Confirms that range query work and return correct result.
    

    This contains grammatical errors. It should be:
    "Confirms that range queries work and return the correct result"

15. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
    @@ -3049,7 +3055,7 @@ class DatabaseBasicSyntaxTestCase extends DatabaseTestCase {
    -   * Test for string concatenation.
    +   * Tests for string concatenation.
    
    @@ -3063,7 +3069,7 @@ class DatabaseBasicSyntaxTestCase extends DatabaseTestCase {
    -   * Test for string concatenation with field values.
    +   * Tests for string concatenation with field values.
    

    The word "for" here either makes "Tests" a noun, or changes the meaning of the sentence. Simply removing the word "for" fixes this.

16. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
    @@ -3102,7 +3108,7 @@ class DatabaseBasicSyntaxTestCase extends DatabaseTestCase {
    -   * Test LIKE query containing a backslash.
    +   * Tests LIKE query containing a backslash.
    
    @@ -3448,7 +3455,7 @@ class DatabaseTransactionTestCase extends DatabaseTestCase {
    -   * Test committed transaction.
    +   * Tests committed transaction.
    

    These need articles (a committed transaction, a LIKE query, etc.) (This likely applies elsewhere in the patch as well.)

17. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
    @@ -3288,12 +3295,12 @@ class DatabaseTransactionTestCase extends DatabaseTestCase {
    -   * Helper method for transaction unit test. This "outer layer" transaction
    -   * starts and then encapsulates the "inner layer" transaction. This nesting
    -   * is used to evaluate whether the the database transaction API properly
    -   * supports nesting. By "properly supports," we mean the outer transaction
    -   * continues to exist regardless of what functions are called and whether
    -   * those functions start their own transactions.
    +   * This is a helper method for transaction unit tests. This "outer layer"
    +   * transaction starts and then encapsulates the "inner layer" transaction.
    +   * This nesting is used to evaluate whether the the database transaction API
    +   * properly supports nesting. By "properly supports," we mean the outer
    +   * transaction continues to exist regardless of what functions are called
    +   * and whether those functions start their own transactions.
    

    This method still needs a one-line summary at the top above this paragraph.

18. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
    @@ -3630,7 +3637,7 @@ class DatabaseTransactionTestCase extends DatabaseTestCase {
    -   * Test transaction stacking and commit / rollback.
    +   * Tests transaction stacking and commit / rollback.
    

    How about, "Tests transaction stacking, commit, and rollback"?

19. +++ b/core/modules/system/tests/modules/database_test/database_test.testundefined
    @@ -3757,7 +3764,7 @@ class DatabaseNextIdCase extends DrupalWebTestCase {
    -   * Test that the sequences API work.
    +   * Tests that the sequences API work.
    

    I think this needs to be "works"? API is singular.

Let's get a reroll to fix these specific points, and to also apply these suggestions elsewhere in the files. Please be sure to include an interdiff from the previous patch. Thanks!

Here's what I did (sorry I can't create an interdiff due to PSR-0ification of tests):

• Cleaned up patch to apply to new PSR-0 tests.
• Applied @xjm's suggestions.
• Reviewed patch manually and fixed errors grammatical and code style errors. However, I believe there are a few outstanding >80 character definitions that I wasn't sure how to summarize further.

Re-rolled.

Thanks for the review @jhodgdon! This patch is a monster.

l) I've actually seen both. Even k) has inconsistencies. Perhaps this should be handled in another issue?

New patch and interdiff attached!

I *think* I got 'em all :)

Fixed!

Backported #26 and #31 to D7.

.