Use {@inheritdoc} on all class methods (including tests)

This looks great. The rationale for this is that we adopted our Test writers guidelines, and the consensus was that overridden methods do not have to be documented.

Committed to CVS HEAD. Thanks.

I'm reopening this issue. It was never tagged "coding standards", and the standard that was adopted for the PHP docs for the tests is not consistent with the rest of our class-docs standards:
http://drupal.org/node/1354#classes

I would like to propose that we:

a) Add getInfo() to the DrupalWebTestCase and DrupalUnitTestCase base classes (it is not on any of the test case classes now that I can see).

b) Make the standard for docs of getInfo() and setUp() consistent with our class docs guidelines. Specifically, they should have an abbreviated documentation header saying:

/**
 * Overrides DrupalWebTestCase::setUp().
 */
function setUp() {
}

(similar for getInfo().

Moving to documentation... But I'm not particular. It's just not AJAX :-)

Sorry about that - must have clicked the wrong component. :)

Not your fault. Whenever a component has been removed or renames, it defaults to AJAX when the issue is updated. I get one of these every couple of days :-)

I think #4 sounds reasonable, and it dovetails nicely with the current sprint. It seems strange to have a standard where we require docblocks everywhere, except also explicitly require not having them in three small cases. That's confusing. I'd prefer the short-form docblocks we use for overridden methods generally. Less to remember. :)

Plus, having them improves code readability and makes it more accessible to novices, I think.

Well, I see two votes for #4, and so far no one else has weighed in. We probably would need a patch for this... (a) is quick. (b) should be scriptable. Sounds like a fun project for this morning... :)

I agree with both @xjm and @jhodgdon that we should be consistent with our documentation standards where ever possible. Other than the nuisance of writing

/**
 * Overrides DrupalWebTestCase::setUp().
 */
function setUp() {
}

at the top of each test class, I do not understand why this is not done. I would encourage this change.

Perhaps it would be even better if we created templates for best-of-breed UnitTest and WebTest class documentation that people could grab when they start to work on a new set of tests? Once this change is resolved, I can try to help with that.

I would also like to edit http://drupal.org/node/325974 (Guidelines page) so that the recommendations there are in accordance with the rest of the docs guildelines on http://drupal.org/node/1354. Besides the class method problems:
- Class docblocks should start with a verb.
- The suggested @file description should have "the" in it ("Tests for the Aggregator module.")
- The sample test functions' docblock columns are mis-aligned, and it would be better if they started with a verb as a better model.

But I'll wait until this guideline is adopted. And another note: it should apply to tearDown() as well as getInfo() and setUp().

Here's a patch that fixes up the docs for setup/teardown, and adds an overrideable function for getinfo to both Unit and Web test cases. I'm working on a script to add the "Overrides" lines to all existing core test classes too.

Sending it to testbot. I think this could help make test writing less of a WTF for novices.

The patch is not done. I'm working on the rest (updating the test files).

Oops, sorry. :)

OK. Here's a patch that includes #12 and also adds docblocks to all of the getInfo(), setUp(), and tearDown() methods.

I generated the docblock stuff with a Perl script and a bash Shell script (I know, PHP here but it's so much easier to do this stuff in Perl...). The script checks to see whether a class extends DrupalUnitTestCase, and if not, assumes it the methods are overridden from DrupalWebTestCase. So there could be a couple of cases where the method doc that is added is saying it's overriding a different method... probably not too many, hopefully?

Scripts attached too, in case anyone is interested. I added .txt to their extensions so they could be attached.

Note: If you want to run the scripts:
- Name them doall.sh and fixtests.pl
- Put them in the same directory.
- Make them executable.
- Run them from that directory (Drupal root, or core or core/modules).

OK, I can already see it didn't catch one DrupalUnitTestCase... will need to try again. What do people think about the concept?

Fixed that problem - here's a new patch and a new script.

Since none of these methods have docblocks currently, this patch should not collide with other patches. Sending to testbot to find out for sure. :)

#19: 338403-19-simpletest-cleanup-with-methods.patch queued for re-testing.

I'll wait on rebuilding this until it has a chance of being committed (critical count down etc.). Any comments on the idea or methodology? Someone else can also feel free to rerun the scripts. Instructions #17, one script #19, other script #16. I guess I'll go edit the issue summary now.

I just edited the issue summary.

Title which reflects the issue's current scope.

.

+++ b/core/modules/file/tests/file.test
@@ -1098,6 +1122,9 @@ class FileTokenReplaceTestCase extends FileFieldTestCase {
 class FilePrivateTestCase extends FileFieldTestCase {
+  /**
+   * Overrides DrupalWebTestCase::getInfo().
+   */
   public static function getInfo() {

@@ -1106,6 +1133,9 @@ class FilePrivateTestCase extends FileFieldTestCase {
+  /**
+   * Overrides DrupalWebTestCase::setUp().
+   */
   function setUp() {

Well, I guess this would make sense if it would actually be correct. ;)

1) DrupalWebTestCase itself does not define getInfo(). As such, test cases can only implement getInfo(). (In turn, and given that a defined getInfo() method is actually required and tested against, and a test case without is straight-out ignored/invalid, DrupalWebTestCase should actually be declared as abstract.)

2) In this case, not DrupalWebTestCase::setUp() but FileFieldTestCase::setUp() is overridden, so the phpDoc here is actually invalid.

20 days to next Drupal core point release.

update to new status of issue

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -646,7 +646,27 @@ class DrupalUnitTestCase extends DrupalTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'Sample functionality',
+      'description' => 'Longer description of the tested functionality',
+      'group' => 'Sample group',
+    );
+  }

@@ -1218,6 +1241,28 @@ class DrupalWebTestCase extends DrupalTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'Sample functionality',
+      'description' => 'Longer description of the tested functionality',
+      'group' => 'Sample group',
+    );
+  }

Sorry, only after rewriting the summary I actually noticed that this patch attempts to add getInfo() methods to the base classes. That's wrong and won't fly.

20 days to next Drupal core point release.

You should reach out to some of the original people who made the decision not to document these—http://groups.drupal.org/node/7731#comment-53456

I'm confused; that seems to be advocating exactly the docblocks we're suggesting here?

Edit: Sorry, I needed to scroll down.

I really don't have time/energy to fight this, but I'm strongly -1 to "documenting" all of this for all the reasons I wrote at that g.d.o post webchick linked to. These comments add no value, generate more visual noise, are wrong most of the time, and are a waste of everyone's time. Just like cut and paste coding is bad, cut and paste comments aren't any better. If we need to replicate an identical comment 100s (1000s?) of times in our codebase, something is wrong.

Furthermore, we don't do this kind of thing, either:

// Add two numbers
$i = 1 + 2;
// Print the results
echo $i;
// Increment the value again.
$i++;
// See if the value is bigger than 6
if ($i > 6) {
  // Do something.
  something();
}
...

We have to assume that people reading Drupal PHP code (at some level) understand PHP. If you don't understand how class inheritance works, these comments aren't going to help you. You need other resources, not our code comments, to gain that understanding.

Finally, most of these comments are technically wrong, since they're documenting methods that don't override the base class, but some other child of the base. So, instead of a script to puke the same comment everywhere, for this to be at all useful, we should document which parent's setUp() every child is actually overriding. But that's a mess, since if you add another class in the hierarchy, you just potentially invalidated a whole bunch of comments further down the tree and you have to go fix all of those. Trying (and inevitably failing) to keep all this in sync via manual effort of updating the comments is nuts. IDEs and other automated methods can tell you what method you're inheriting or overriding. We have vastly more important things to spend human effort on.

I'd move this back to "remove pointless comments on inherited methods" and set it to closed (fixed), but it's not up to me to make that decision unilaterally.

I agree with the points @dww outlined. Based on that, we rather need to clarify the documentation standards on class inheritance, so that this case is unambiguously covered.

Regarding getInfo(), I've created #1339054: DrupalTestCaseInterface to properly require getInfo()

I can confirm that figuring out what's overriding or implementing whom is time-consuming and frustrating at times. However, that's precisely why I feel that having an (accurate) docblock for these and other child class methods generally is important. Not everyone uses an IDE, and the short docblock is really, really helpful when reading unfamiliar code. It may take one person some time to add them, but how many dozens of other people then have saved that time? They make the code easier to understand, which makes Drupal more accessible. I also don't think they're anything near an // Increment $i. ;)

For me, a docblock at each function or method also makes the code easier to scan, especially since that's what we have everywhere else.

Well, we had a huge discussion a while back on documenting class methods that are overrides, and decided every function and method should have a docblock. We didn't have a discussion about documentation standards for tests (at least, a discussion that involved documentation folks previously)...

So I don't get it... Are sun/dww advocating for removing all inherited method docblocks, or just in tests? If just in tests, why are they different from other classes?

AFAIK the previous standard was to remove it from all inherited methods, or at least that's the stick I applied with DBTNG, etc. I'm not sure where it was discussed changing that.

#718596: Lacking standards for documenting classes, interfaces, methods seems to indicate the opposite, though. It requires the short-form docblocks for inherited methods.

Yeah, that is what we decided on that other issue. I still don't understand why tests are an exception to the general standard?

I feel that the @jhodgdon's proposal in #4 is appropriate (there shouldn't be an exception), especially in context of #1518116: [meta] Make Core pass Coder Review and #1545476: false positive "Missing function doc comment" error on Simple Test inherited methods. Is it appropriate to create a new issue specific to the standard?

Thanks for pinging this issue -- we really need to decide this.

Once again: Can someone please explain to me why the Test classes should be an exception to the general rule that all class methods and properties should have a documentation block?

Other OO-based projects solve this through a special {@inherit} phpDoc comment.

I'm not 100% (perhaps 80%) arguing for that, just pointing out that it kinda resolves the documentation issue as well as the code maintenance issue.

I don't see {@inherit} in the PHPDoc documentation, just {@inheritdoc}, and I don't see how that helps us:
http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria...
That specifically says it is used to inherit the long description of a class from the parent class. It doesn't seem to apply to methods. Where's this {@inherit}?

Also, just to point out that for people not on api.drupalorg and not using IDEs, our system of saying:

/**
 * Overrides WhateverClass::methodName().
 */
function methodName() {}

gives you more information than if the docblock just said:

/**
 * {@inherit}
 */
function methodName() {}

Also, this docblock is not necessary... We could easily make the API module inherit doc blocks when none are provided. The question is whether we want to have *all* class methods/properties have documentation, and I still think we do.

The question is whether we want to have *all* class methods/properties have documentation, and I still think we do.

I've been a fan of this for a long time, but recent addition as well as refactoring of code to OO revealed that the enforced phpDoc on implemented as well as overridden class methods is not really maintainable.

It's an OO language feature that you can simply change extends Foo into extends FooBar, or even extends Drupal\Foo\Bar, or even extends Bar (whereas Bar has been imported via use Drupal\Foo\Bar; upfront).

The enforced phpDoc for implemented and overridden methods complicates that language feature. Instead of doing the simple, obvious thing of changing, say, extends DrupalWebTestCase into extends DrupalUnitTestCase, and be done with it, you've to grep the entire file for potentially stale phpDoc blocks and make sure that they're updated, too.

Even more so, you actually have to grep the entire code base, in order to make sure that the phpDoc of methods in any classes that may inherit from the class that you're changing is updated accordingly, too. You never know whether that is the case.

Most often, the other instances are simply forgotten. And you don't catch them in patch reviews either, because they are not part of the diff context.

Lastly, this documentation standard is also incompatible with Drupal's modular design. The module in project A may extend class A now, and a module in project B may have a class B that extends class A. We currently ask class B to document that it extends class A and implements/overrides certain methods of class A, given their fully qualified name. But when class A is changed to class C, or to extend a generic base class D, then the entire documentation on class B becomes just simply stale, bogus, and invalid. As long as class B still works as intended, there'd never be any reason to look into class B in project B. Hence, the documentation would never ever be updated to reference the actual/correct class names that are actually inherited/implemented/overridden.

I am in favor of documenting implemented/overridden methods, because I'm not working with an IDE, so I've no idea whether a method may be declared in an interface or defined in the parent class. However, my recent experience with core patches clearly shows that they are just simply impractical.

If I understand #41, the proposal would be to make our documentation standard something like this (to replace what's currently in http://drupal.org/node/1354#classes, which will also need revised examples):
---
All new (not overridden/implemented) class members (methods, properties, and constants) should have documentation blocks. Overridden and implemented class members (including the implicit methods setUp(), tearDown(), and getInfo() for SimpleTest test classes) should not have documentation blocks unless there is a need to say something beyond the documentation that is already on the member they are overriding or implementing (for example, to clarify what is different between this method and the parent method that is being overridden).
---

And the idea would be to make sure that the API module inherits the documentation from the method that is being overridden/implemented (and inserts a line somewhere saying it's an override/implementation of another method).

Did I capture the essence correctly? If so, I am OK with this standard, as I understand the reasoning, even though it's not so great for people trying to understand the code by reading just the files (not on api.drupal.org or in an IDE). Any other thoughts/votes (assuming I understood the idea correctly)?

Correct, though I actually meant all classes, not only tests. The problem space is not limited to tests. But that's probably a different issue.

Yep, that's the basic gist.

And while the "100% completeness" part of me agrees with what you say, in practice it's actually nice to be able to totally ignore those functions when they're in my IDE because I know they're inherited and aren't doing anything special. The trick, of course, is to make sure that those are the *only* functions without PHPDoc, so it's seen as deliberate and not a mistake. ;)

I like the idea of {@inherit} or something explicit that lets me know "Nope, on purpose; not a mistake." But it's very obtuse.

RE #43, the standard proposed in #42 deliberately covers all classes, and says that test classes should be treated the same way as other classes. I think/hope?

RE #44, we can put in an @inherit if you like... although I think that the API module currently would correctly handle #42 without the @inherit, and it would need some new code to handle it.... So perhaps something like this:

// @inheritdoc
function whateverItIs() {
}

That way CoderReview/Sniffer/Etc. would have something to recognize, and API could ignore it. :)

scope creep. :)

FYI: We're starting to run into this problem... #1495024-76: Convert the entity system to PSR-0 updated phpDocs of all classes and methods that people didn't think of in the actual change.

OK... so we have a proposed standards change in #42, with a slightly different proposal in #45. Any thoughts on what the standard should be?

The only thing I know, // @inheritdoc won't fly it needs to be inside a proper docblock, sorry.

What exactly do you mean by "won't fly"?

The API module will handle it correctly -- it will ignore the // @inheritdoc comment, and it already has the feature that methods that completely lack documentation inherit documentation from the parent class.

Thus the // @inheritdoc is there for human doc readers/writers who want to have something there to remind them not to write a full doc header and/or to look to the parent class for documentation.

I just saw this issue for the first time.

I believe what @chx meant is // @inheritdoc would not work, but the following would:

/**
 * {@inherit}
 */

What *I'm* saying is that // @inheritdoc *will* work, and at least from the point of view of the API module, it's actually much better than putting it into a /** */ doc block (as the API module maintainer, I think I can speak with authority on that).

What I want to know is why chx thinks it won't work.

Unassigning until we have consensus on what to do.

I kinda wish we would un-scope creep this back to the original issue title of making getInfo()/setUp()/tearDown() consistent, and move the @inheritdoc stuff elsewhere, since that's a larger discussion.

The inconsistency with these "three-off" no-documentation rules consistently comes up as a barrier for new Drupal core contributors. They get informed about the docs gate when their patches are marked needs work, then proceed to meticulously document each function in their patches, then get a COUNTER-needs work to remove documentation from these oddball functions. Case in point: #1943624: EntityMalformedException when enabling a text editor while creating a new text format, because Editor depends on FilterFormat And no great explanation as for "why" other than "Some people argued about it one time and overruled common sense." :P~

Just as a note, I do not believe (at least in D7, not sure for D8) that there even is a getInfo() method on the base test classes, so @inheritdoc would not even work for those.

Also, there is only tearDown() and not setUp() on TestBase in D8 currently. There's a separate issue for adding getInfo():
#1999512: getInfo() should be an abstract method with docs on TestBase/UnitTestCase

related #1869794: Update tests coding standards doc and make consistant with 1354 where appropriate

Now that we have adopted @{inheritdoc} we should fix the Simpletest coding standards: https://drupal.org/node/325974

Oops, it is {@inheritdoc}, not @{inheritdoc}.

Updated issue summary.

update for {@inheritdoc}

One problem with that idea is that there is no documentation to inherit on most getInfo() methods, since the base test classes do not include this method.

Also, just to be clear... the API module doesn't really care whether you have a missing docs header or {@inheritdoc}, and I think also that most IDEs don't care either. However, for consistency I agree it would be a good idea.

Can we add getInfo() to the base class as an abstract method or whatever makes it so the sub-classes have to override it? (hand-waving) That'd allow the use of @inheritdoc.

I have definitely suggested that before, but have been told it would screw something up... I have no idea.

Moving to the simpletest.module component in hopes that the maintainers can enlighten us. I will also update the issue summary so that this question is highlighted.

re-arranged for clarity

Meh, PHP does not allow me to declare a method "abstract public static", so we need to throw a run time exception.

This patch should be the foundation so that we can use {@inheritdoc} in a meaningful way on getInfo()/setUp()/tearDown().

Hm, so we need to catch that exception when listing test classes to filter out abstract base classes, or we could use reflection to check if it is abstract.

That looks like a reasonable code change to me.

The documentation needs a minor update: when making a list, capitalize the first word after the : for each list item. See https://drupal.org/node/1354#lists for more info.

I am also not sure that the getInfo() list documentation is very clear:

+   *   - name: describes an overview of what subsystem is tested by the class;
+   *     for example, "User access rules" or "Menu link creation/deletion".
+   *   - description: One sentence describing the test, starting with a verb.
+   *   - group: human-readable name of the module (Node, Statistics), or the
+   *     human-readable name of the Drupal facility tested (e.g. Form API or
+   *     XML-RPC).

- In "name": describes an overview... => An overview... ... and I don't really think we need to say "subsystem", how about just "An overview of what the class tests"? And does it really need two examples?
- In "description", could you make it clearer the distinction between name and description? Maybe it needs an example or at least something saying it's longer than "name"?
- In "group", start with "the", and please please please do not use "e.g." because (as here) it is always misused or mis-punctuated. Just say "for example,".
- Can we have consistency on quotes? Either enclose all the examples in single-quotes, or all in double-quotes, or none in quotes.

I just found UnitTestCase::getInfo(). It would be cool if we could adapt the Exception message there so that they match.

If UnitTestCase already had a getInfo(), then why do we need the changes in this patch for simpletest.module?

Because UnitTestCase in contrast to UnitTestBase does not extend TestBase. It's the PhpUnit one, whereas UnitTestBase is the Drupal-y not really unit-test one. It's super confusing, but that's how it currently is.

Oh. Hm. I hope the class docs explain all of this. :)

Thanks for the feedback, tried to address all comments.

I also copied the @return documentation to the phpunit UnitTestCase, since the example there was bad.

First line in the patch needs a comma I think:

Provides meta information about this test case such as test name.

Also, can we make sure that the getInfo doc matches in the two base classes?

Other than that, I think the docs look pretty reasonable.

Sure.

The docs look good to me. I'll let others set to RTBC (code, etc.).

+++ b/core/modules/simpletest/simpletest.module
@@ -480,7 +480,14 @@ function simpletest_test_get_all() {
+          try {
+            $info = call_user_func(array($class, 'getInfo'));
+          }
+          catch (\RuntimeException $e) {
+            // A run time exception indicates that this is an abstract base
+            // class, so we skip it.
+            continue;
+          }
 

As a follow up it would be cool to actually show the error message to the users that they have missed the getInfo implemetation. As klausi explained we need this at the moment to not fail in the abstract base class case.

Patch no longer applies.

Rerolled, no other changes.

Showing messages that implementers forgot a getInfo() method is neither happening before nor after this patch, so that would be a separate feature request.

#77: simpletest-inheritdoc-338403-77.patch queued for re-testing.

+++ b/core/modules/simpletest/simpletest.module
@@ -480,7 +480,14 @@ function simpletest_test_get_all() {
         if (class_exists($class) && method_exists($class, 'getInfo')) {
-          $info = call_user_func(array($class, 'getInfo'));
+          try {
+            $info = call_user_func(array($class, 'getInfo'));
+          }
+          catch (\RuntimeException $e) {
+            // A run time exception indicates that this is an abstract base
+            // class, so we skip it.
+            continue;
+          }

Checking whether the class is abstract is the better approach, because this catches and ignores any kind of RuntimeException; i.e., it hides real errors.

Using Reflection for that sounds sensible.

Alternatively, you can also introduce an exception class that is specific to Simpletest and only catch that.

Applied Reflection to detect abstract classes.

#80: simpletest-inheritdoc-338403-80.patch queued for re-testing.

Let's try this one...

Oops, now I can reproduce the error from the previous bad patches...

Tags

I'm not convinced we should use reflection here, but since the results get cached anyway there should not be much performance harm.

Looks RTBC to me!

+++ b/core/modules/simpletest/simpletest.module
@@ -480,8 +479,22 @@ function simpletest_test_get_all() {
+            // Avoid to test intermediate classes which don't implement method.

fixing this grammarwise.

I read through the rest of the docs and they all look good. I double checked against some examples of getInfo()

took out an unrelated whitespace change.

Thanks!

+1

Committed to 8.x. Thanks.

Great, I updated the Simpletest coding standards to use {@inheritdoc}. Could somebody review that as well? https://drupal.org/node/325974

Should we try to backport this to D7?

The new docs look fine to me.

I am not sure about porting this to Drupal 7. It's probably not necessary?

The problem is that our coding standards are always current, so they also apply to D7. Using {@inheritdoc} for the getInfo() method in D7 is not really correct, since there is no parent method in the code that would have the documentation.

But I'm fine with ignoring D7 at this point :-)

I agree that inheritdoc is not really correct for Drupal 7. You could add a note to the Simpletest standards doc if you want. The API module won't care if inheritdoc is there and there is nothing to inherit, though -- it will act the same as our previous standard of not putting any docs at all on the methods.

I think #1999512: getInfo() should be an abstract method with docs on TestBase/UnitTestCase can be closed.

Yep, marked that issue as fixed.

add note about getinfo