[policy, no patch] Discuss the standards for phpunit based tests

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.

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

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.

Editing the issue summary now...

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

Is it time to add the documentation tag?

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

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.

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.

Added --strict

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.

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

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

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

@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

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

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.

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:

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

Linked to @covers documentation.

Shall we make an exception to say provider methods dont need docs?
Or say what docs they need to have?

Right now we have a variety that is strictly conforming, and some without any:
ag -B10 "public function provider" core/*
gives a sample

some have no docs at all, some have a oneline summary, some have an @returns with little info, some have @returns which specifies each element of the array, and some have a @see to the test they provide data to.

a.

90-  public function testFormatBacktrace($backtrace, $expected) {
91-    $this->assertSame($expected, Error::formatBacktrace($backtrace));
92-  }
93-
94-  /**
95-   * Data provider for testFormatBacktrace.
96-   *
97-   * @return array
98-   */
99:  public function providerTestFormatBacktrace() {

b.

565-   * Data provider for testFilterXssAdminNotNormalized().
566-   *
567-   * @see testFilterXssAdminNotNormalized()
568-   *
569-   * @return array
570-   *   An array of arrays containing strings:
571-   *     - The value to filter.
572-   *     - The value to expect after filtering.
573-   *     - The assertion message.
574-   */
575:  public function providerTestFilterXssAdminNotNormalized() {

c.

100-  /**
101-   * Provides data for ConfigMapperManager::testHasTranslatable()
102-   *
103-   * @return array
104-   *   An array of arrays, where each inner array contains the schema element
105-   *   to test as the first key and the expected result of
106-   *   ConfigMapperManager::hasTranslatable() as the second key.
107-   */
108:  public function providerTestHasTranslatable() {

In the summary, it says:

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

But:
$ ag "public function provider" core/* | wc -l
129
$ ag "public function providerTest" core/* | wc -l
106

So maybe we should change that?

Where possible, name data provider methods the same as their test method counterpart, prepending with provider. For example: testSuperCoreCanInject() would have a data provider of providerTestSuperCoreCanInject().

So maybe we should change that?

Where possible, name data provider methods the same as their test method counterpart, prepending with provider. For example: testSuperCoreCanInject() would have a data provider of providerTestSuperCoreCanInject().

Maybe even provideTestSuperCoreCanInject, which also solves #21 and is nice to read :) but maybe too verbose

Shall we make an exception to say provider methods dont need docs?
Or say what docs they need to have?

I would say providers need docs, because they are helpful, in the future when one goes too add more test cases. They know what is what and in which order it should be.

What i find useless is @param docs for test methods. Noone will ever use/call them but phpunit...and if you really want to know you can check the data provider's @return docs

As far as @return, there's this other discussion happening: #2253915: [policy, no patch] Fix coding standards for PHPUnit tests

I say we should document @return in the provider, with a bullet-list of the array elements, and then not duplicate in @param in the test method. sun wants to not document anything. :-)

Also +1 on #37.

people is there any resources for testing modules stand alone? i have worked on this i think is a pretty good example without mocking every core class https://github.com/dmouse/vimeo_field

no, i guess i meant to test a module standalone, that is to have a container mocker of the services so that they can be easily tested.
The functional test you spoke of is a real container with all the services in. This is different. I say in addition to what you are saying we should do this, since in a test suite for a module we should have both types of tests. But definitely everything should be standalone and not tested with the framework when they are installed.

I believe all this strategy has to be revamped strongly. One comment I want to make is trying to do one assertion per test method whenever possible.

There are many tests that can be improved, and we need to start driving development with tests rather than writing tests as a second step. Especially with core maybe there should be a guideline?

@cordoval: No, you can't currently test a module's integration without a Drupal to slot into.

You can, however, unit-test all the classes in your module.

Good luck turning the ship of Drupal dev to be test-driven. I mean that both sincerely *and* sarcastically. :-)

Reading the latest revision in the issue summary, I'd like to raise some concerns + suggestions on the Documentation chapter:

1. Use @group Drupal to allow filtered testing.

   Specifying @group Drupal does not make sense to me, because only tests belonging to Drupal are discovered in the first place.

   In other words, you are not able to run non-Drupal tests through the regular phpunit test suite to begin with. Therefore, adding @group Drupal sounds pointless to me.

   I also haven't seen such a @group in phpunit tests of any other application framework yet. The reason seems logical to me: If you don't want to run Drupal tests, then you should not run the Drupal test suite.

2. Use @group Namespace or similar for further filtering. e.g: @group Entity

   This could be more explicit. I'd like to see the following wording:

   For tests of components of Drupal core, the test class MUST specify the (capitalized) component name as the first @group. For example, @group Routing. Additional groups MAY only be specified if the test class MUST be executed as part of the secondary groups, too, but that only happens rarely for component tests.

   For tests of modules, the test class MUST specify the internal module name as the first @group. For example, @group entity_reference. Additional groups MAY be specified, if applicable.

   For PHPUnit tests, every specified @group MUST reference an existing primary @group; i.e., either the name of a Component, or the internal name of a module. PHPUnit @group annotations MUST NOT invent new/custom group names that do not map to any other component or module.

3. Use @see to link to the class which is tested.

   An additional @see is needless clutter in ~99% of all cases. I'd like to see the following wording:

   Each PHPUnit test class SHOULD specify the class under test with @coversDefaultClass. Given a @coversDefaultClass annotation, it is RECOMMENDED to omit the PHPDoc summary line, since the summary would only duplicate the information stated by @coversDefaultClass.

   If a PHPUnit test class does not cover a single class (integration tests), the test class MUST have a PHPDoc summary line and it is RECOMMENDED to specify @see tags for each involved class.

Thoughts?

In case anyone has any issues with the suggested adjustments to the proposed best practices for documenting PHPUnit tests in #47, please speak up now. :)

#697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit) is about to implant exactly that practice into all PHPUnit test classes of Drupal core.

Aside from that, let's also discuss #2297541: [policy, no patch] Secondary/additional test @group names (ideally focused, over there)

#697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit) is pretty awesome. No more, "What can I make up to put in the description field?"

If a PHPUnit test class does not cover a single class (integration tests),

An integration test isn't a unit test, so it should be marked @coversNothing.

Cool. We need to incorporate #47 and #49 into the proposed docs in the issue summary. I'll try to do that later today.

That said, I'd prefer to change the wording in #49 from "SHOULD" into "RECOMMENDED". It's OK to be explicit and specify @coversNothing, but the implicit meaning of omitting it should also be fine.

Coding standards changes are now part of the TWG

In light of #2328919: Remove () from a bunch of @covers definitions in PHPUnit I added a bit about not using parentheses in @covers.

Should we have @uses ?
See #2371531-10: Expand unit testing for Drupal\Component\Plugin\PluginManagerBase
https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi...

If we allow multiple @covers per test method (which we apparently do), then we don't really care enough to have proper @uses.

You can see where you'd need @uses by doing this: ./vendor/bin/phpunit --strict --coverage-text -v

Here's some sample random output:

411) Drupal\Tests\Core\Utility\UnroutedUrlAssemblerTest::testAssembleWithLocalUri with data set #5 ('base://example', array('example'), true, '/subdir/example#example')
This test executed code that is not listed as code to be covered or used:
- /Users/paulmitchum/projects/drupal8/core/lib/Drupal.php:111
- /Users/paulmitchum/projects/drupal8/core/lib/Drupal.php:112
- /Users/paulmitchum/projects/drupal8/core/lib/Drupal/Component/Utility/UrlHelper.php:281
- /Users/paulmitchum/projects/drupal8/core/lib/Drupal/Component/Utility/UrlHelper.php:282
- /Users/paulmitchum/projects/drupal8/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php:38
[... and a lot more ...]

This means there's a discrepancy between whether, for instance, \Drupal is covered by this test. That's because UnitTestCase::setUp() always uses \Drupal to set the container to NULL, for every test method.

One solution is to say @uses \Drupal at the beginning of this test.

Another is to not use UnitTestCase if you don't need that behavior.

And still another solution is to not care. :-)

What say ye, o Drupal communiteee?

Todo add @depends to the documentation.

Remove @see. We have @coversDefaultClass and @covers for this.

@dewehner: I know where it is for. I just reviewed your patch on #2386195: State has no dedicated test coverage. I thought it would be a good idea to add it to the standards.

@dahwehner: @Mile23 You are quite script about test coverage. I would fear that making resul to strict results in no written phpunit tests at all.

Not really strict about test coverage. I like to make coverage reports in order to find out what needs some unit testing love, and if I can't generate one it makes me sad. So sad.

@daffie Todo add @depends to the documentation.

Here's what I would write: "Avoid @depends. It reduces isolation between tests."

What is the policy for assert messages? I was under the impression it should be the failure message, rather than what *should* happen. The tests in core seem to support that.

For instance, in \Drupal\Tests\Core\Site\SettingsTest

 /**
   * @covers ::get
   */
  public function testGet() {
    // Test stored settings.
    $this->assertEquals($this->config['one'], Settings::get('one'), 'The correct setting was not returned.');
    $this->assertEquals($this->config['two'], Settings::get('two'), 'The correct setting was not returned.');

    // Test setting that isn't stored with default.
    $this->assertEquals('3', Settings::get('three', '3'), 'Default value for a setting not properly returned.');
    $this->assertNull(Settings::get('four'), 'Non-null value returned for a setting that should not exist.');
  }

The documentation page doesn't mention assert messages.

I think many of the tests use the other convention, and that is what I've always thought it was (from past experience writing tests for other software).

But I actually think if you polled the Core maintainers they would advocate having no message at all (using the default message, which would say something like "A equals B" or "A is null".

By the way if you look at the default messages for the assert* methods I think you will see they are the "this passed" message, not the "this failed" message.

we dont need a policy for this.
this is why: https://www.drupal.org/node/2002190#comment-7494804
that's the correct way to use phpunit assertion messages.

Just a heads-up about this... #2415441: Automate finding @covers errors

It's both a documentation issue and a code-quality one.

#2462037: [policy, no patch] Discuss usage of @codeCoverageIgnore seems related

Should we be using @test and not prefixing the test with the name test?

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

Some of the concerns of this issue came up on #2915490: Modernize & harden HalLinkManagerTest. Specifically data providers. The thought from that issue is (quoting @Wim Leers) that:

Any class that extends \PHPUnit\Framework\TestCase should no docblock for any method whose name has the prefix provider.

Reading the issue summary it is clear that it needs a major update. Also trying to set all the standards at once might be super tricky. So I've opened a specific issue for data provider standards in unit tests.

Now that WebTestBase is deprecated in Drupal 8 https://www.drupal.org/docs/develop/coding-standards/drupal-simpletest-c... seems even more out-of-step and harmful.

We should at least put a note at the top of it that it's only for Drupal 6/7?

@jhodgdon it all looks a bit messed up atm there's Drupal 8 stuff through the document. One thing that bothers me about the page is that it's not really a coding standard. It blurs the lines into a howto. And this is classic:

Test template

After the opening PHP tag and a blank line comes the @file declaration:

/**
 * @file
 * Contains \Full\Namespace\Of\The\TestClassName.
 */

If there is no namespace (Drupal 7 and previous), omit that.

Because our coding standards in Drupal have long since removed the @file docblock.

@ParisLiakos (#64)

we dont need a policy for this.
this is why: https://www.drupal.org/node/2002190#comment-7494804
that's the correct way to use phpunit assertion messages.

We absolutely do need a policy for this.
If not, people will look at the many examples in core where we do it wrong, and assume this is a convention in Drupal.
And once we have a policy, that policy can justify an effort to clean this up.