Remove the requirement for doxygen for test methods

+1000.

We should instead encourage self-documenting test method names.

Yes. Of course yes.

Every exception to the "everything needs documentation" rule makes the "everything needs documentation" rule harder to understand. It does not take much time to write one line describing what a test does, and some of the method names are really not that easy to figure out what is being tested. They're either overly long (which is HardToParseInCamelCaseAndGetsTiringToRead), or overly short to the point of cryptic. I say, keep the one line requirement -- it's not like they have params or return to deal with.

Properly setting title/tags for a coding standards discussion.

it's not like they have params or return to deal with.

Actually for unit tests that use data providers that's not true. The actual data providers have these huges arrays as return values, and the actual test methods have the respective array values as parameters.

I was recently asked to document all that on an issue, and it felt ridiculous at first. But then I realized that unless you've written the test yourself the return value of data providers are very hard to grok, so I guess in case tests have to be amended or refactored in the future, it might actually be helpful.

So I'm a bit torn on this issue...

I was recently asked to document all that on an issue, and it felt ridiculous at first. But then I realized that unless you've written the test yourself the return value of data providers are very hard to grok, so I guess in case tests have to be amended or refactored in the future, it might actually be helpful.

Given how many arrays you have on data providers I totally agree.

Let's have a look at an existing test and see what is not really needed (don't give additional information).

  /**
   * The test request.
   *
   * @var \Symfony\Component\HttpFoundation\Request
   */
  protected $request;

  /**
   * The mocked access manager.
   *
   * @var \Drupal\Core\Access\AccessManager|\PHPUnit_Framework_MockObject_MockObject
   */
  protected $accessManager;

  public static function getInfo() {
    return array(
      'name' => 'Local tasks manager.',
      'description' => 'Tests local tasks manager.',
      'group' => 'Menu',
    );
  }

  /**
   * {@inheritdoc}
   */
  protected function setUp() {
    parent::setUp();

    $this->request = new Request();
    $this->accessManager = $this->getMockBuilder('Drupal\Core\Access\AccessManager')
      ->disableOriginalConstructor()
      ->getMock();

    $this->setupLocalTaskManager();
  }

Given that, I would propose to keep the @var lines as this adds autocompletion and potentially refactoring support.
For sure this file is inconsistent but I would like at least allow people typehing both so you get autocompletion
for both methods.

The suggestion here is to remove the requirement for documenting methods. When a method is at all unclear, regular coding practices still apply - add a comment.

RE #8... You are talking about a "standard" that would look like

Most of the time you must have a documentation block on all classes, functions, and methods. However, if you are writing a test class, you can sometimes leave the documentation block off a test method. For this to be acceptable, the method must have a self-documenting name, and it must not have any return value or parameters.

A "standard" like this will just lead to bikesheds about whether a given test name gives enough information, and to people abusing it. A standard that says "Everything needs at least a one-line doc block", on the other hand, is not a huge burden for developers, and is clear. And as I stated in #3, I am not really in favor of trying to work an entire line of documentation into a veryLongStringOfDifficultToReadForHumansCamelCase(). I don't see why it's a big deal to ask for a one-line documentation block, really?

For these reasons, I am 100% against this proposal.

A "standard" like this will just lead to bikesheds about whether a given test name gives enough information, and to people abusing it. A standard that says "Everything needs at least a one-line doc block", on the other hand, is not a huge burden for developers, and is clear.

That's not a bikeshed, that's a meaningful discussion about how well our code communicates it's intent.

It's not a given that it will be abused. This is why we do extensive code reviews. Encouraging reviewers to actually read what the code is doing instead of running dreditor on it to find a missing one-line doc is a *very* good thing.

Also, it *is* a burden to developers. If I have to write a comment that reiterates what my method name says (of which we have *many*), it feels cumbersome because all I'm doing is adding noise to the code. There is now one more thing that would need to change if the test changes, and I'm sure we've all seen cases of comments that make no sense because they weren't changed with the code or copy/pasted from another file.

While it's easy to argue that this requirement for test methods is not useful, I'd like to argue that it's harmful to our code. An example:

class HtmlAttributesTest extends WebTestBase {

  /**
   * Tests that attributes in the 'html' and 'body' elements can be altered.
   */
  function testThemeHtmlAttributes() {
    $this->drupalGet('');
    $attributes = $this->xpath('/html[@theme_test_html_attribute="theme test html attribute value"]');
    $this->assertTrue(count($attributes) == 1, "Attribute set in the 'html' element via hook_preprocess_HOOK() found.");
    $attributes = $this->xpath('/html/body[@theme_test_body_attribute="theme test body attribute value"]');
    $this->assertTrue(count($attributes) == 1, "Attribute set in the 'body' element via hook_preprocess_HOOK() found.");
  }
}

Here, the method's comment is the only thing that tells you what this test is actually doing, because the test method name just reiterates the class name. If a test like this comes up in an issue, no one gives it any thought because the requirement has been fulfilled. Now, when I see that this test is failing in a report, I have no idea what it's actually trying to test without opening this file and reading the comment.

I would have written it like this:

class HtmlAttributesTest extends WebTestBase {

  function testHtmlAndBodyAttributesAreAlterable() {
    $this->drupalGet('');
    $attributes = $this->xpath('/html[@theme_test_html_attribute="theme test html attribute value"]');
    $this->assertTrue(count($attributes) == 1, "Attribute set in the 'html' element via hook_preprocess_HOOK() found.");
    $attributes = $this->xpath('/html/body[@theme_test_body_attribute="theme test body attribute value"]');
    $this->assertTrue(count($attributes) == 1, "Attribute set in the 'body' element via hook_preprocess_HOOK() found.");
  }
}

Yes, the test name is longer, but long method names are *not* something we should avoid. That method name communicates exactly what it's intent is. Putting a comment on that would just be noise. Granted, the "And" in there is awkward, but that's because we have a bad habit of putting multiple tests in one method to save the cost of setup (instead of just having multiple methods with a class-level setup).

There are cases where the method name on some of our tests would be ridiculous, but that is a very important code smell! It's telling us that that method is doing way too much (the word "And" is a classic indication of an SRP violation). If we're allowed to give the test a throwaway name with a long comment, we're left with complex methods that are hard to read and debug.

Another example:

class PageCacheTest extends WebTestBase {
  /**
   * Tests support for different cache items with different Accept headers.
   */
  function testAcceptHeaderRequests() {
    $config = \Drupal::config('system.performance');
    $config->set('cache.page.use_internal', 1);
    $config->set('cache.page.max_age', 300);
    $config->save();

Not only is the method name unclear about what it's testing, but the comment is a bit awkward as well.

What it could be:

class PageCacheTest extends WebTestBase {

  function testVariesByAcceptHeader() {
    $config = \Drupal::config('system.performance');
    $config->set('cache.page.use_internal', 1);
    $config->set('cache.page.max_age', 300);
    $config->save();

Who would argue that that method needs a comment? I certainly can't imagine a bikeshed over that.

We should be striving for clear, communicative method names, that do not require comments, but our current standard of a one-line requirement disincentivizes this best practice. It's a problem throughout our entire codebase, but removing the requirement for tests is a very sensible place to start.

#10's first example nicely illustrates exactly my point. IMO,

 /**
   * Tests that attributes in the 'html' and 'body' elements can be altered.
   */
  function testThemeHtmlAttributes() {

is vastly preferable to

 function testHtmlAndBodyAttributesAreAlterable() {

with no docs. The first one is much more readable to me. I dont' like trying to read camel case. Plus, on api.drupal.org, the former will display much nicer. Functions without docs do not do well.

The second... well yes the comment is a bit redundant but it did not I think take the developer more than a few seconds to write, so what's the big harm?

I fail to see any benefit from this standard, and I see a lot of down sides.

The first one is much more readable to me. I don't like trying to read camel case.

I don't think anyone would argue that regular sentences aren't more readable than camel-case, but you're still left with a meaningless/misleading method name. That's bad.

Although we may not be used to it, this is the style that is widely used. Here's some examples from the first popular projects I could think of:

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Htt...
https://github.com/laravel/framework/blob/master/tests/Routing/RoutingMa...
https://github.com/doctrine/doctrine2/blob/master/tests/Doctrine/Tests/O...
https://github.com/django/django/blob/master/tests/validation/tests.py#L12
https://github.com/rails/rails/blob/master/railties/test/generators_test...

Even PHPUnit's tests use this style:

https://github.com/sebastianbergmann/phpunit/blob/3.7/Tests/Framework/As...

So I don't think the readability argument is very compelling.

The second... well yes the comment is a bit redundant but it did not I think take the developer more than a few seconds to write, so what's the big harm?

It's not redundant, it's just completely ambiguous about what it's testing. That's bad, and no one would catch that in a code review because it has it's one-line comment.

And it's not about the time it takes to write the comment, it's about making developers do something that's useless. Everytime I use Drupal, I'm annoyed that I have to put @file on classes and one-line comments where none are needed. This makes working on Drupal unpleasant, especially if you have a background in any other language or framework. I don't want to be annoyed when I'm working on Drupal.

I, too, feel like our documentation standards are too strict. For example, the use of @file at the top of each file seems unnecessary (in Drupal 8) and actually degrades the developer experience. Yes, outside the scope of this issue.

/**
 * @file
 * Contains \Drupal\language\LanguageAccessController.
 */

I can see that, but again, standards with exceptions are harder to understand and follow than standards that say "always".

As much of a fan of consistency as I am, and as much as I hate "oh, except for" rules, I have to say I do find the linked references in #13 fairly compelling. One of the goals of using tools like PHPUnit is to remove "Drupalisms" and make the project more approachable to people who already have familiarity with these libraries. If the best practice out there, from PHPUnit itself no less, is to writeSelfDocumentingMethodNamesThatShowUpInTestResultOutputWithoutFurtherReading() and not put documentation on them unless they do something special like throw an exception or take a var/return value, on the surface at least it seems like we'd be better off to follow suit. Hm.

If we allow this for test methods, why not allow it for everything then?

Why would we want to do it for everything? I don't see anyone proposing it.
Test methods are to be small and single purpose, with names that are self-expanatory and verbose. You never need to use those methods elsewhere.

Real runtime methods need documentation because they actually do something and are used and interacted with by developers and the rest of the system.

This issue is about test methods.

OK.

Someone has recently proposed this also for "magic" methods like __construct() on a separate issue, but agreed we can keep this issue to just be about test methods.

So, can someone propose clear wording to add to node/1354 to describe this proposed addition to the standards?

Coding standards decisions are now supposed to be made by the TWG

Moving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.

Marking Needs Work in agreement with @jhodgdon's comment:

So, can someone propose clear wording to add to node/1354 to describe this proposed addition to the standards?

Can we please do this?

I'm seeing RTBC patches with test coverage that has a protected helper method that only make sense for that one given test getting un-RTBC'd because it doesn't have the necessary test coverage. For example:

+++ b/core/modules/editor/src/Tests/EditorSettingsValidationTest.php
@@ -0,0 +1,111 @@
+  protected function setMaxDimensions($width = '', $height = '') {
+    $edit = [];
+    $edit['editor[settings][plugins][drupalimage][image_upload][max_dimensions][width]'] = $width;
+    $edit['editor[settings][plugins][drupalimage][image_upload][max_dimensions][height]'] = $height;
+    $this->drupalPostForm(NULL, $edit, t('Save configuration'));
+  }

I could have simply not moved this into a helper method, and you'd have more repetitive, more brittle test code. And I wouldn't have been nagged by this rule to document every single method. Because I'd have written a worse test, and thus avoided this painful, silly requirement.

Note that this issue's title explicitly says for test methods. Not for all methods. Just for tests!

Therefore I propose to revise https://www.drupal.org/coding-standards. Apparently https://www.drupal.org/coding-standards#comment points to https://www.drupal.org/node/1354 which points to https://www.drupal.org/node/1354#tests which points to https://www.drupal.org/node/2500059 (for SimpleTest) and https://www.drupal.org/node/2116043 (for PHPUnit). That's a separate issue though.

I think it can be very simple:

Every method MUST be documented, EXCEPT methods in tests. The only methods in tests that are required to be documented are methods that are helper methods to be used by many tests. For example: \Drupal\simpletest\AssertContentTrait::assertRaw(), \Drupal\simpletest\AssertContentTrait::cssSelect().

We also need to ensure that we can create the rule properly in coder. One of the things that makes this tricky are abstract test bases where documentation is helpful.

I'd agree that 90% or more of test method docblocks are totally redundant, as in the methods that specifically begin with the word test. Not other methods on test classes.

However, I see a few counterpoints that I'd want to make sure we address:

1. I agree with alexpott that we should still require them when the method is on a base class (the less common pattern we use where one base class contains test methods that are run for each extending child test).
2. Many test methods also have names that are not helpful. If we remove the docblocks, we should make sure the method name itself is specific and descriptive, even if it is very long. E.g. testFoobarFormValidationWithUnprivilegedAccount() rather than testForm(). This verbosity is an OK tradeoff since test methods very rarely get used/called outside their declaration. People would need to be more attentive to the actual test method names when they submit a patch. The kneejerk "missing docblock" feedback would be replaced with "method name too vague" in a lot of cases.
3. In some cases, it is helpful to have extended documentation that explains details about the test beyond what would be in a method name. We should still allow/require docblocks for those cases.
4. "A docblock is always required" is more consistent/learnable than "A docblock is required in circumstances X, Y, and Z, but not in A, B, or Q". Some also find that the docblocks improve scannability since they are used to seeing them above everything in Drupal.

I think these tradeoffs are probably OK, but I want to make sure they are addressed. I would narrow the scope of Wim's proposal to:

Every method MUST be documented, EXCEPT test case methods (e.g., MyTestClass::testFooBar()) when the method name itself sufficiently documents the method's purpose. If more information is needed to describe the the test case, a docblock should still be added. All other methods in test classes (helper methods, assertions, fixtures, etc.) still require documentation.

+1 to #24.

Thanks @jhodgdon -- I updated the summary to that.

+1 to most of #24, but -100 for the helper methods exception.

Please re-read #22. That clearly provides an example of where it's sensible to have a helper method, but senseless to document it. In other words: you've narrowed the scope so much that it limits the reduction of unnecessary work significantly. If you do this, then you actively encourage repetition of code rather than adding helper methods for a particular test class.

I propose simplifying it to:

Every method MUST be documented, EXCEPT methods in tests. Of course, when valuable, a docblock should still be added. The only methods in tests that are required to be documented are methods that are methods to be used by many tests: assertions and helper methods in traits and base classes.

Another problem with not requiring docblocks for test* methods is that for proper unit tests it should be required that they have an @covers annotation.

I like that the proposal from @xjm in #24 said that you could only drop the doc block if the method name fully documented the method. #27 is missing this idea.

I think I'm more -1 to this proposal.

• A normal sentence describing the purpose of a test method is much more readable than a verbose method name in camelCase.
• Considering that test methods almost never have parameters or return values, the additional effort is very small. Just write the method name again, as a proper sentence.
• As xjm writes, we should generally be reluctant to add exceptions to general rules. I don't think there's enough benefit here to justify the reduction of clarity.
• IDEs probably won't support this, at least not for a while, and you'd get complaints for missing doc blocks even when we allow it in our coding standards.

In any case, I'd definitely be against Wim's extended proposal. What he describes in #22 doesn't seem at all related to tests, it just seems like not wanting to document a small helper method. Which, true, might make sense in some cases, where the doc block is tricky to write and adds almost nothing – but that case is almost impossible to quantify. "Always write doc blocks unless you're really sure you don't have to." is a terrible rule.

@ Wim: I understand your frustration at having a patch rejected because of this requirement, but that really is no basis for changing this standard.

I am personally undecided about the requirement for possibly redundant human-language text in the doc comment.
I do, however, strongly feel that we should always have @param and @return on any method that does have parameters or a return value - including test cases.
This is the case for test methods with data providers (see #6 (tstoeckler), #7 (dawehner)), and it is the case for helper methods as in #22 (Wim Leers).

Otherwise I support the points by xjm (#24) and drunken monkey (#30).

A lot of bugs in Drupal are because developers did not think about all the possible data types or value ranges a variable can have. E.g. a variable could be NULL, but the code was not prepared for it.
The IDE is already flashing all colors of the rainbow, so the real bugs don't stick out.
Code like this is also harder to read and follow. Imo, the type hints are often more important than human-readable text.
And since PHP is not a strongly typed language, and native type hinting is often incomplete, the type information should be in the doc comment.

For my taste, test case methods with obvious names, or with names that follow a well-understood pattern, do not absolutely need a human-language text, if this does not add any extra information. But then again, I understand the consistency argument, which jhodgdon is defending in e.g. #15, and the scannability argument xjm mentions in #24 (doc blocks as a decorative element above everything).

@jhodgdon (#19):

Someone has recently proposed this also for "magic" methods like __construct() on a separate issue, but agreed we can keep this issue to just be about test methods.

Indeed. On constructors, the human-language comment is quite worthless, except for scannability. Maybe just write "The constructor.", so we can copy this everywhere, and don't need to verify if it has the correct class name. But yes, this is a separate issue.

@msonnabaum (#13):

you're still left with a meaningless/misleading method name. That's bad.

Yes. If we could improve method names, possibly stick to a consistent pattern, we can drop the human-language comments.
But maybe improving the method names should come first, not the other way round?

It's not redundant, it's just completely ambiguous about what it's testing. That's bad, and no one would catch that in a code review because it has it's one-line comment.

And it's not about the time it takes to write the comment, it's about making developers do something that's useless.

Yes.
Useless and redundant comments are not just annoying for a developer to write, but also a candidate for reviewers to pay less attention to. So it is possible that poor, redundant or invalid comments are committed.
As a reader, you may get used to skipping the comments altogether, because you experience that half of them are not worth reading, or even incorrect.

Sometimes I think we should just put an empty comment, if we don't have anything meaningful to say. This way we have the aesthetic / scannability effect, but readers understand they can skip it, and don't waste their time reading something with no information value.
<?php
/**
* -
*/
function foo() {..}

The rest of your comments in #10 and #13 is great. But I think it depends if we can get used to making up better method names.
Maybe we need a separate issue "Naming conventions for test methods".

<hr/>

@Wim Leers (#22)
I don't find your example convincing.
<code>
+  protected function setMaxDimensions($width = '', $height = '') {
+    $edit = [];
+    $edit['editor[settings][plugins][drupalimage][image_upload][max_dimensions][width]'] = $width;
+    $edit['editor[settings][plugins][drupalimage][image_upload][max_dimensions][height]'] = $height;
+    $this->drupalPostForm(NULL, $edit, t('Save configuration'));
+  }

I did not read the issue where this patch was posted, so I could be wrong (you did not post a link).
But from the code snippet, I do not think this method should go without documentation. The method name suggests this is a setter method - e.g. for a property on the test class instance. Seeing a drupalPostForm() call in there is not something I expect from the method name.
And what about the parameter types? Are width and height integer, or floats, or string numbers? With 'px' appended, or without?

For code I write myself (contrib modules, libraries, code for client projects), a lot of the methods don't have human-language comments. But this is ok only when method names are meaningful, and/or all the information is already clear from the @param and @return type hints.
(they still have @param and @return and friends, and all of them are IDE-friendly (depending which inspections are enabled).

@jhodgdon (#15):

I can see that, but again, standards with exceptions are harder to understand and follow than standards that say "always".

I think this is a blessing and a curse at the same time.
It is easy for reviewers to address standards compliance first, and architecture second. So three versions of a patch with minor code style fixes may be submitted, until someone actually talks about architecture, or understandability. The more important concern should be "The purpose of this method is unclear to a reader.", not "This method lacks a human-language doc description.".

I am still undecided which conclusion I should draw from this :)

@param and @return for new code or any test being updated are now covered by type hinting.

This does not cover (sorry) @covers but I think we could allow only @covers in that case.

Helper methods - if they're on an individual test, no docs for me. If they're on a base test class or trait, they should have docs anyway.

added new issue summary template

Added myself as a supporter and adjusted the proposed wording change in the issue summary.

Move the current and proposed text to expected place in the issue summary.

I suggest the following.

All classes and all of their methods (including private methods but excluding constructors) must be documented. 

This does not apply to test methods on concrete test classes. Such methods should have a self-documenting method name. When the name is not sufficient documentation should be added.

What sniffs would no longer be applied to tests?

Converted to an MR and updated credit.

I think the wording here makes sense, adding myself as supporter as well. I think we have enough supporters if we count the older comments on this issue.

Adding myself as a supporter.

In the converting to an MR I changed the text. The meaning should not have been changed but can someone confirm the MR is correct?

There are 3 supporters, next is a change record.

The text in the MR seems clear to me to understand the rule and intent to still document where it is needed.

Created a change record, and I agree with @quietone's latest remark as well, if we can make this change this should be good to be discussed at the next coding standards meeting.

I think the suggestion by @longwave looks very good. Do you want to accept this proposal @quietone?