Problem/Motivation

Part of meta-issue #1310084: [meta] API documentation cleanup sprint

Proposed resolution

Correct the following in the Test base classes in core Simpletest module:

  • Ensure that each file has an @file docblock.
  • Ensure that all @return lines are preceded by a blank line.
  • Ensure that @see and @ingroup lines are always at the end of docblocks.
  • Ensure that all lines in doxygen blocks are 80 characters or fewer (excluding the terminal newline).
  • Ensure that all functions, classes, interfaces, and methods have one-line summaries that are clear, use the correct verb tense, and follow specific standards in http://drupal.org/node/1354.
  • Make incidental style and grammar corrections only to those docblocks already being updated.

Remaining tasks

Patch needs to be rolled.

User interface changes

None.

API changes

None.

Follow-up issues

None.

CommentFileSizeAuthor
#1 1805264-1-testbase-docs.patch84.22 KBLars Toomre
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Component: simpletest.module » documentation
Assigned: Lars Toomre » Unassigned
Status: Active » Needs review
FileSize
84.22 KB

There is much clean up required to make the Test base classes in the Simpletest module conform with D8 documentation standards. Examples include:

  • Missing @param directives,
  • Extraneous @param directives,
  • Incorrect verb tense in one line method summaries,
  • Many missing '(optional)' text strings in variable descriptions.
  • Many missing explanations of what the default value of a variable was, and
  • Some number of lines not wrapping correctly for 80 characters.

Where I was uncertain of a value or string, I used '???' to indicate that something needs to be supplied before this patch can be committed. Let's see what the bot thinks of this untested first patch for this issue.

jhodgdon’s picture

Status: Needs review » Needs work

Please confine this patch to API documentation -- no code lines. Thanks!

Aside from that, most of the patch looks good! A few things to fix up:

a)

+   * @param string|bool $status
+   *   Can be 'pass', 'fail', 'exception'.
+   *   TRUE is a synonym for 'pass', FALSE for 'fail'.

Wrapping -- this can all go on one line.

b) Maybe this information should be left in the first line (if it fits):

/**
-   * Check to see if a value is not false (not an empty string, 0, NULL, or FALSE).
+   * Checks whether a value is not FALSE.
+   *
+   * A 'not false' value is not an empty string, 0, NULL, or FALSE.

Same with the following function... Having it in the first line will help when scanning the method list. If you do need to move it, at least make the capitalization consistent ('not FALSE').

c)

* @param $message
-   *   The message to display along with the assertion.
+   *   (optional) The message to display along with the assertion. Defaults to
+   *   an empty string.
    * @param $group
-   *   The type of assertion - examples are "Browser", "PHP".
+   *   (optional) The type of assertion - examples are "Browser", "PHP".
+   *   Defaults to 'Other'.

Let's leave these changes out for now. There's a separate issue that's discussing wholesale changes to how the $message/$group parameters should be documented so it would be better to do it there:
#1457320: Document that t() should not be used for assertion message texts and groups

d)

+   * This method backups various current environment variables and resets them,

Backup is not a verb. It should be "backs up".

e)

/**
    * Constructor for UnitTestBase.
+   *
+   * @param string $test_id
+   *   (optional) The test ID.  Defaults to NULL.
    */
   function __construct($test_id = NULL) {

Should only be one space after . in this line. There's another one at

+   *   equeal.  Hence, a return of zero means that both the file size and file
+   *   names are equal.
    */
   protected function drupalCompareFiles($file1, $file2) {

f)

/**
-   * The value of the Drupal.settings JavaScript variable for the page currently loaded in the internal browser.
+   * The value of the Drupal.settings JavaScript variable for the page currently
+   * loaded in the internal browser.
    *
-   * @var Array
+   * @var array
    */
   protected $drupalSettings;

Needs one-line summary.

g)

/**
-   * The original user, before it was changed to a clean uid = 1 for testing purposes.
+   * The original user.
+   *
+   * This value is saved before it was changed to a clean uid = 1 for testing
+   * purposes.
    *
    * @var object
    */
   protected $originalUser = NULL;
 

There is a verb tense agreement problem in the new paragraph (is/was). The same problem is in the next few doc blocks.

h) Take care of the ??? in the patch.

That's about where I lost steam... it looks like this patch isn't done.

Lars Toomre’s picture

Component: documentation » simpletest.module

Moving to simpletest component because although it primarily is a documentation patch, the patch in #2 attempts to bring some rationalization to use statements at top of the three base test files. Apparently changing code and documentation in the same patch is inappropriate for a documentation patch.

jhodgdon’s picture

Component: simpletest.module » documentation

Um. We need a docs cleanup patch for the API Docs Cleanup effort. Please just provide a patch that doesn't mess with code. Thanks!

Lars Toomre’s picture

I will leave it to someone else to roll such a patch.

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Closed (won't fix)

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!