Updated: Comment #11

Problem/Motivation

Discussion and guidance are needed on the issue of coding standards for PHPUnit-based tests.

Proposed resolution

Write up some standards, have a review period, and make a handbook page.

Remaining tasks

Reconcile these against the SimpleTest coding standards: https://drupal.org/node/325974

Discuss pros and cons of various standards.

Add the documentation tag.

Proposed PHPUnit Coding Standards (thus far)

Drupal core PHPUnit-based tests must:

Organization:

  • Exist inside the /core/tests directory.
  • Have a classname that is the name of the class under test, plus 'Test.' For instance: The class SuperCoreInjectable would be tested by SuperCoreInjectableTest.
  • Live in the same namespace as the class under test, plus \Tests. Thus: Drupal\Core\SuperStuff\Tests\SuperCoreInjectableTest
  • Each test class must reside in its own file, discoverable under the standard PSR-0/PSR-4 system for Drupal: /core/tests/Drupal/Core/SuperStuff/Tests/SuperCoreInjectableTest.php

Documentation:

  • Use @group Drupal to allow filtered testing.
  • Use @group Namespace or similar for further filtering. e.g: @group Entity
  • Use @coversDefaultClass and @covers where appropriate to prevent false positive test coverage reports. @covers documentation
  • Only use @covers to reflect methods called from within the test method. That is, if you assert the result of \Foo::bar() you can't claim coverage of \Foo::privateMethodThatBarCalls()
  • Use @see to link to the class which is tested.

Best Practices:

  • Tests must pass under --strict. This means, for instance, that all tests must throw an exception.
  • Isolate tests as much as possible, using a rule of thumb that each method being tested should have its own test method. Test methods were expensive in SimpleTest; in PHPUnit they're cheap.
  • Use annotation features of PHPUnit as much as possible. These include @dataProvider, @expectedException, and so forth.
  • Always use a @dataProvider method if there is any data for testing. This allows us to separate concerns, so that we don't have to refactor tests to accommodate data, only change the data.
  • Where possible, name data provider methods the same as their test method counterpart, removing the Test and prepending with provider. For example: testSuperCoreCanInject() would have a data provider of providerSuperCoreCanInject(). This is a guideline, and it may make more sense to have a readable method name rather than one that starts with provider*. Strive to be consistent within the test class, at least, so that it's easy to pick the provider methods from a list of methods.
  • Use @expectedException where possible.
  • Mock as much as possible (using PHPUnit's getMock()).
  • Use sentence-style test method names, such as testFrammistatWithGoodData() and testFrammistatForException(), to make code readable, and to help various reporting systems provide useful output. (See ./vendor/bin/phpunit --testdox for details.)
  • When possible, use reflection to gain access to private and protected methods for testing. This allows you to perform the test without creating a stub subclass.
  • When using reflection to gain access to private and protected methods is unfeasible or impractical, test-scope-only subclasses of classes under test should be named Stub plus the name of the class. Example: StubSuperCoreInjectable extends SuperCoreInjectable {}. These subclasses can be declared in the same file as the test class which uses them, since they should be small, minimal implementations.

Comments

Some ideas for the standard i have in mind:

  • A single test file per tested php class
  • The test file should have the same classname + "Test", so Foo and FooTest and should also be in the same namespace beside of Tests
  • The test should mock everthing beside of the actual tested class
  • Use @group in the class documentation to group in phpunit
  • Use @see to link to the class which is tested
  • Use as many as possible test functions, to split up testing different methods on the class
  • Use @dataproviders where possible. The naming scheme for it should be providerTestFunctionsName
  • Use @expectedException when possible

I mostly agree with these.

The test file should have the same classname + "Test", so Foo and FooTest and should also be in the same namespace beside of Tests

We should aim for this, but I think it's also ok to have higher level acceptance-style tests that test the interaction of a number of classes, in addition to the unit test per class. I wouldn't want to exclude tests like this.

The test should mock everthing beside of the actual tested class

When possible. If other classes are more or less decoupled from any infrastructure-type dependencies, I think it's fine to just use them. Mocking everything can get crazy at times.

Use as many as possible test functions, to split up testing different methods on the class

I don't quite understand this.

Use @dataproviders where possible. The naming scheme for it should be providerTestFunctionsName

I think it's important to emphasis that the main benefit of @dataproviders is that it allows you to dynamically create tests. Meaning if you don't use them, if the first one fails, none of the other cases get run. Sometimes this behavior is appropriate, sometimes it's not.

We should aim for this, but I think it's also ok to have higher level acceptance-style tests that test the interaction of a number of classes, in addition to the unit test per class. I wouldn't want to exclude tests like this.

Yeah we could add them as well. For example all the routing components of core should be tested like that.

When possible. If other classes are more or less decoupled from any infrastructure-type dependencies, I think it's fine to just use them. Mocking everything can get crazy at times.

Yeah for things like the request or the DIC I hate to mock them, as it needs quite some time.

Use as many as possible test functions, to split up testing different methods on the class

I don't quite understand this.

On simpletest we used to avoid test functions, as this really cost time. This is not really the case anymore with phpunit.

I think it's important to emphasis that the main benefit of @dataproviders is that it allows you to dynamically create tests. Meaning if you don't use them, if the first one fails, none of the other cases get run. Sometimes this behavior is appropriate, sometimes it's not.

Afaik there are not many tests which would have problem with that behavior. Mostly it is about throwing a lot of different options onto an object/method.

Issue tags:+phpunit

You want to use @dataProviders as much as possible because it makes it much easier to change/add to the data while keeping the logic separate. Even for just one or two data sets it's easier to write readable test method code if you don't have a bunch of arrays hanging around, and if someone has to add data to your test they won't need to refactor it.

Use as many as possible test functions, to split up testing different methods on the class

I'd add: Use sentence-style test method names, like controllerCanReturnJsonTest(). To see one reason this is useful, type ./vendor/bin/phpunit --testdox

As far as mocking: Yes, when possible, mock. (Insert Nelson ha-ha! here.) Dependencies are bad. For testing the interaction between two classes, you'd mock one and test the second, and then mock the second and test the first, and then test them both in unmocked form. How to mock: http://phpunit.de/manual/3.7/en/test-doubles.html

@group is different from @defgroup. This will no doubt lead to confusion.

Drupal standards for @defgroup: https://drupal.org/coding-standards/docs#defgroup

PHPUnit @group: http://phpunit.de/manual/3.7/en/appendixes.annotations.html#appendixes.a...

Also, contrib should use the module name for @group, and core should use @group Drupal. It would be really great to be able to run all the core tests in one go with ./vendor/bin/phpunit --group Drupal.

You can have more than one @group, so other groups could reflect namespaces and APIs, such as @group Drupal_Component. Contrib could do similar things, such as @group modulename_namespace.

Speaking of PHPUnit and contrib... I'm writing phpunit_example in the Examples project. Swing on by and criticize. #2032697: Create PHPUnit example module

You can have more than one @group, so other groups could reflect namespaces and APIs, such as @group Drupal_Component. Contrib could do similar things, such as @group modulename_namespace.

This is awesome! It would be great to have both full drupal as well as based upon component as you suggested.

I'd add: Use sentence-style test method names, like controllerCanReturnJsonTest(). To see one reason this is useful, type ./vendor/bin/phpunit --testdox

Oh I thought every function has to start with "test" and not end with?

Oh I thought every function has to start with "test" and not end with?

Ugh. yah. :-)

testControllerCanReturnJson() :-)

#2051717: Expand phpunit tests for \Drupal\Component\Utility\Unicode turns providers to static
we should also agree on a pattern there to be consistent..RIght now we only have one static provider in core

Turning providers to static was probably an oversight on my part. If they aren't required to be static, there are probably some advantages to keeping them dynamic.

Well, they are not called statically, so I don't see a point in making them.

What about trying to collect the information and create a handbook page now?

Assigned:Unassigned» Mile23

Editing the issue summary now...

Assigned:Mile23» Unassigned
Status:Active» Needs review

OK, I summarized the discussion so far and put it in the issue summary.

Is it time to add the documentation tag?

When using reflection to gain access to private and protected methods is unfeasible or impractical, test-scope-only subclasses of classes under test should be named Sub_ plus the name of the class. Sub_SuperCoreInjectable extends SuperCoreInjectable {}. Overridden methods should also use the Sub_ prefix so everyone knows what is going on. These subclasses can be declared in the same file as the test class which uses them, since they should be small.

I am not sure about that, though I like the point of using the same file. This really helps to understand why this is actually needed. Phpunit does some magic with the autoloader anyway, so no reason to set strictness on PSR0.

It's not really magic with the autoloader. It just always includes files that have PHPUnit tests in them.

But I have a new thing to think about: @requires and/or markTestSkipped().

I'm thinking of \Drupal\Component\Utility\Unicode, which behaves differently depending on which extensions are enabled. We could do some logic in the tests to skip the cases where those extensions or methods aren't available, which means those parts of the code could be tested in other environments.

Skipped tests still result in an overall pass, so no problem there.

The problem is that some folks might freak out if they see SSSS in their test output. :-)

Issue tags:+coding standards

More voices please. :-)

We might decide on using phpunit strict="true", at which point a bunch of existing tests will be marked as incomplete because they don't have assertions.

You can see this behavior by running ./vendor/bin/phpunit --strict

Should it be a coding standard that all tests have assertions?

i can see the usecase of having just a test to something just to make sure there are no fatal errors?
although it makes sense to always have an assertion i dont think we should enforce it

#16 and #17, I think we should generally use --strict. Here's an issue: #2096595: Make all PHPUnit tests pass with --strict

You can see that 2 were just named wrong, and the other two could have had better assertions.

Issue summary:View changes

Updated with actual stuff at comment #11.

So I'll take that as a yes, tests should pass with --strict, which generally means having to throw an assertion.

Updated the issue summary.

Issue summary:View changes

Added --strict

Component:simpletest.module» phpunit
Issue summary:View changes

Please, let's not require the "provider" prefix, it doesn't read correctly.

testSuperCoreCanInject reads nicely because it's a statement. providerSuperCoreCanInject does not read well because that makes no sense grammatically.

I'd prefer no standard here at all because it's really unnecessary, but if we must, it should be a suffix, like superCoreCanInjectProvider.

+1 for not requiring it in general. Tests are "just" tests, and should not require a ton of knowledge about our codestyle, people should just be able to write them.

test-scope-only subclasses of classes under test should be named Sub_

I'd prefer 'Fake' instead of Sub_. Makes more sense IMHO.

Issue summary:View changes

'Stub*' would be more specific. 'Mock*' is probably out, to avoid confusion with getMock() objects. Also underscores are bad.

As far as 'provider*', I'd say be consistent within the test class, if you can't be consistent throughout Drupal. It should be clear which methods are providers in that class by looking at a list of method names.

Updated.

Martin Fowler gives his definitions here: http://martinfowler.com/articles/mocksArentStubs.html

There was some support in irc for "it is nice to" type hint the type of the thing that might be being mocked.

See \Drupal\user\Tests\PermissionsHashTest
for example

  /**
   * A different account.
   *
   * @var \Drupal\user\UserInterface|\PHPUnit_Framework_MockObject_MockObject
   */
  protected $account_2;

Also, tests use a lot of chaining so #1516460: Define coding standard for chained method calls might be related.

Issue summary:View changes

@coversDefaultClass can be put on the test class, so that when referencing specific @covers methods later, we dont have to repeat a long namespace
@covers is used to prevent a false reporting of test coverage on code that "just happens" to be called during a test.

http://phpunit.de/manual/3.8/en/appendixes.annotations.html

An example (whipped up from tim)

<?php
namespace Drupal\foo;
class
Foo {
public function
thing() {
  return
$this->thang(4);
}
protected function
thang($num) {
  return
$num + 2;
}
/**
* @coversDefaultClass \Drupal\foo\Foo
*/
class FooTest extends UnitTestCase {
/**
* @covers ::thang()
*/
public function testThang() {
 
$foo = new Foo();
 
// We cannot call thang() directly because it is protected.
 
$this->assertSame(6, $foo->thing());
}
?>

---------

also taking out the @ingroup phpunit recommendation. tim said we didn't need it and jhodgdon said we should not use @ingroup phpunit also because we dont have a @defgroup for it.

and https://api.drupal.org/api/drupal/core%21tests%21Drupal%21Tests%21UnitTe...
would give someone who wanted to know all the phpunit tests since they extend UnitTestCase

Issue summary:View changes

@covers and @coversDefaultClass look good. They'll make coverage reports more scary, though, with less tertiary coverage being reported.

The example in #27 doesn't actually test thang(), however, so using @covers ::thang() would be incorrect. You'd say @covers ::thing(). So maybe that's the rule: You can only use @covers for methods you actually call in the test.

On the one hand, you're not supposed to test private/protected methods like thang(). On the other hand, you can do the reflection shuffle to get thang(), or make a stub class and unit test it. (I like the reflection shuffle, in the case of Drupal, since there's so much legacy code behind the APIs at this point.)

The example in #27 doesn't actually test thang(), however, so using @covers ::thang() would be incorrect. You'd say @covers ::thing(). So maybe that's the rule: You can only use @covers for methods you actually call in the test.

This is simply pointless, as this won't catch protected methods.

On the one hand, you're not supposed to test private/protected methods like thang(). On the other hand, you can do the reflection shuffle to get thang(), or make a stub class and unit test it. (I like the reflection shuffle, in the case of Drupal, since there's so much legacy code behind the APIs at this point.)

isn't that the wrong approach? This would make it basically impossible to refector internal code. You would assume internals if you care about the names of the protected methods.

isn't that the wrong approach? This would make it basically impossible to refector internal code. You would assume internals if you care about the names of the protected methods.

Well, if the protected method is worth covering, then write tests that directly cover it. If it's not, then don't claim you're covering it. @covers should be for methods you're really covering, not the other methods being called from that one.

If you never write tests for hidden methods, then ideally you can refactor all day long without changing any tests since your existing tests define the public behavior only.

But if you care about test coverage of hidden methods, you'll end up re-writing tests anyway when you refactor. If you have tests that explicitly cover hidden methods, and they're separate from tests that cover the public methods, then the tests are still isolated from each other and this makes the task easier.

Imagine refactoring and ending up deleting thang() above. Now you have a test of thing() that claims to cover a nonexistent method. Did the test's author only call thing() in order to cover thang()? Which part of the test is valid now?

Did the test's author only call thing() in order to cover thang()?

Yes, that's why they put @covers ::thang(). And running with --code-coverage would fail the test.

I disagree with everything @Mile23 said in 28 and 30.

I disagree with everything @Mile23 said in 28 and 30.

Yeah you should write tests about what the code should do, not about how the code looks like internally. This might work in a business enviroment where you don't refactor anything anyway, though this is really not the case for drupal core.

Yes, that's why they put @covers ::thang(). And running with --code-coverage would fail the test.

PHPUnit 3.7.31 by Sebastian Bergmann.
unrecognized option --code-coverage

@covers does only one thing: It tells the coverage reporter to ignore any code run for this test that isn't in the @covers part. In our example above, if you generate a coverage report, our test which calls thing() in order to cover thang() will only report coverage for thang(). That's all it does. http://phpunit.de/manual/3.7/en/appendixes.annotations.html#appendixes.a...

If you're calling thing() in order to test thang(), then you're writing a bad unit test, regardless of how it's annotated. Instead, you should make a stub or reflection object, call the protected thang() that way, and then you will really, actually cover thang() in the test.

Yeah you should write tests about what the code should do, not about how the code looks like internally. This might work in a business enviroment where you don't refactor anything anyway, though this is really not the case for drupal core.

Apparently I'm being misunderstood. In the mythical world where you don't refactor, you'd do what's suggested in #27. In an iterative development process, you do the next guy a favor by doing something like this:

<?php
namespace Drupal\foo;
class
Foo {
  public function
thing() {
    return
$this->thang(4);
  }
  protected function
thang($num) {
    return
$num + 2;
}
/**
* @coversDefaultClass \Drupal\foo\Foo
*/
class FooTest extends UnitTestCase {
 
/**
   * @covers ::thing()
   */
 
public function testThing() {
   
$mockFoo = $this->getMock('Drupal\foo\Foo');
   
$this->assertSame(6, $mockFoo->thing());
  }
 
/**
   * @covers ::thang()
   *
   * @todo: Add dataProviders
   */
 
public function testThang() {
   
$mockFoo = $this->getMock('Drupal\foo\Foo');
   
$class = new \ReflectionClass($mockFoo);
   
$method = $class->getMethod('thang');
   
$method->setAccessible(true);
   
$this->assertSame(6, $method->invokeArgs($mockFoo, array(4)));
  }
}
?>

I think we should put some actual examples in the issue summary, to draft what the doc suggestions are.

For example, does it matter if () are in the @covers?
should it be
@covers ::thang()
or
@covers ::thang

or is either fine?

http://phpunit.de/manual/3.7/en/appendixes.annotations.html#appendixes.a...
shows @covers ::thang

Issue summary:View changes

Linked to @covers documentation.