Docblocks for setUp(), getInfo(), and tearDown()

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:

(similar for getInfo().

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

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.

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

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.

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.

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

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.

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

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.

Title which reflects the issue's current scope.

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.

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 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:

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 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.

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.

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