One SimpleTest template to rule them all
public
It stands to reason that the tests in simpletest/tests will be the ones that people will use to base their own tests. These tests were written by at least 5-6 different people, and it shows. As a result, it's really confusing for a new developer to get a sense of what your own module's SimpleTest integration ought to look like.
Thanks to the hard work and good comments to everyone here, this page has been moved to drupal.org and promoted as our "official" Test writers guidelines.
First off, great document!
First off, great document! We should add a link to this in the Coding Standards or Best Practises section of the handbook on drupal.org.
My only question is in relation to the Doxygen
@filecomments. Why should these be omitted in test files? Why have a different coding standard compared to other PHP files? If this is going to be the standard for test files, then it definitely should be documented on http://drupal.org/node/1354 in the handbook. Has anyone thought through how omitting these @file comments will affect the generation of the API documentation on api.drupal.org?Cheers,
Stella
Agreed...
I updated the doc to reflect this. Thanks, Stella!
Another thing I saw as I was updating that was this sentence:
Which I'm pretty -1 about. This is going to look goofy once API module starts parsing class definitions. I get that you don't want to duplicate the stuff in getInfo(), but what if we came up with a simple convention for all classes like:
/**
* TestCase for ..uhh... someting... hm.
*/
Anyone have any ideas? ;)
The @file description
The @file description doesn't need to be too long. You could just say something like
Test cases for foo moduleor if they're related to a particular include file you could specify the file name. I feel thegetInfo()description should go into more detail.Also, I presume that the API module can still retrieve function level comment blocks for functions that reside in a class file, so I would recommend getting into the specifics there. The example in the document above is confusing. I think something like the following would be good:
/*** Tests creation of user. (One line description only!)
*
* More detailed description of the different tests and assertions carried out to be described here.
* Can go over multiple lines as needed.
*/
Cheers,
Stella
"Implementation of foo" comments are evil
In IRC just now, I was arguing with webchick and Crell that this comment just adds 3 lines of code to the file, makes 3 less lines of code fit on the screen of your editor without scrolling, and adds ZERO helpful information to let people understand the code:
<?php/**
* Implementation of getInfo().
*/
function getInfo() {
...
?>
In fact, webchick initially said (in passing): "That way, I know it's an implementation of a hook", but in fact, this isn't a hook at all, it's a function defined in a base class that this child class has to define. ;) So, the comment is actually doing more harm than good to help people understand.
Either we need to use comments that add value/understanding, or we shouldn't comment them at all. Since I think I'm going to be completely outvoted on "don't comment them at all if you're just saying "implementing a function called 'foo'", then we should re-write these to say something useful. Some options to consider:
A)
<?php/**
* Define metadata for this test subclass.
*/
function getInfo() {
...
?>
B)
<?php/**
* Define metadata for this test subclass by implementing the base class's getInfo() function.
*/
function getInfo() {
...
?>
C)
<?php/**
* Implementation of the base class's getInfo() function.
*/
function getInfo() {
...
?>
Could be something else, too. But, for the sake of the kittens, please don't leave it as "Implementation of foo()". ARGH! I'll have to go kill the kittens myself if I see those all over the place. ;)
Thanks,
-Derek
A++
Think I like A the best. Short and sweet, but a better description. Don't like B for the fact that it will have to be repeated...often.
I agree with dww that saying
I agree with dww that saying nothing here is best. The only reason we started doing "An implementation of hook_foo()" is so that api module could do its backreferences thing. Until we have some better solution to that, we should continue to do this for hooks. As mentioned, this is not a hook.
As I ranted last night.. ;)
One of my goals is for D7 to ship with zero holes in its API documentation, so we need to have /something/ there.
I'm cool with A too, and have updated the template accordingly. Thanks!
Someone want to roll the easiest patch to commit ever? :)
I think that's totally
I think that's totally misguided. The getInfo() is going to be on EVERY test class. It's hardly a surprise what it does after you see it the first time. There's little chance of accidentally confusing it with anything else. In core it make sense because there's no way of knowing if a function is supposed to implement a hook or just named that way. In the tests that's not the case, it just adds visual noise to every class. I think that every test function case should have a comment though, those are the important ones.
Update: I realized that i didn't explicitly say that i think we should have NO comment on the getInfo(), setUp() and tearDown() functions. I don't think it serves any purpose.
Yay, the voices of reason are growing louder... ;)
Excellent -- so far we've got dww, moshe and drewish all on the side of sanity and reason here. ;)
@webchick: can you please reconsider your position that every single function in all of core needs a (potentially useless and noise-generating) PHPdoc comment? I think it's quite a stretch to classify the member functions of every unit test class the core "API".
I totally agree that each test class needs a nice comment (not because those are the core API, either). However, comments on the individual member functions (especially the ones inherited from the base class that are always found in every test class and are completely self-documenting in their names are just silly. Adding comments for the sake of having 100% "complete" comment coverage is not a worthy goal if it's going to add zero useful information and a lot of noise. Please, let us keep our API documentation high in signal and low in noise.
Thanks,
-Derek
and a patch
I agree with the no-comments-on-inherited-member-functions advocates.
Another small argument is that if the need for these comments is to generate the documentation, it would be possible to leave out the comments on inherited functions, but inherit the documentation from the parent function, so that we still get 100% documentation coverage.
Anyway, I created an issue to remove all comments on inherited versions of getInfo(), setUp() and tearDown(): http://drupal.org/node/322197
Oh you guuuyyyysss... ;)
FINE.
I suppose I can see the argument of inherited ones.
But let's make sure all testX functions are PHPDoced as well as the "mother" setUp/tearDown/etc. functions.
+1 for option A
+1 for option A
Time to move!
Ok, we discussed this well, it's now time to move it to drupal.org.
Here are our "Test writers guidelines": http://drupal.org/node/325974
Damien Tournoud
http://drupalfr.org