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(). @covers for methods should not use trailing parentheses. So @covers ::foo and not @covers ::dontDoThis()
  • 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 make an assertion.
  • 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

dawehner’s picture

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
msonnabaum’s picture

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.

dawehner’s picture

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.

ParisLiakos’s picture

Issue tags:+phpunit
Mile23’s picture

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

dawehner’s picture

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?

Mile23’s picture

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

Ugh. yah. :-)

testControllerCanReturnJson() :-)

ParisLiakos’s picture

#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

jhedstrom’s picture

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.

dawehner’s picture

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?

Mile23’s picture

Assigned:Unassigned» Mile23

Editing the issue summary now...

Mile23’s picture

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?

dawehner’s picture

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.

Mile23’s picture

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

Mile23’s picture

Issue tags:+coding standards

More voices please. :-)

Mile23’s picture

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?

ParisLiakos’s picture

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

tim.plunkett’s picture

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

Mile23’s picture

Issue summary:View changes

Updated with actual stuff at comment #11.

Mile23’s picture

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.

Mile23’s picture

Issue summary:View changes

Added --strict

Mile23’s picture

Component:simpletest.module» phpunit
Issue summary:View changes
msonnabaum’s picture

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.

dawehner’s picture

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

kim.pepper’s picture

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

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

Mile23’s picture

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.

kim.pepper’s picture

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

YesCT’s picture

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.

YesCT’s picture

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

Mile23’s picture

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

dawehner’s picture

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.

Mile23’s picture

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?

tim.plunkett’s picture

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.

dawehner’s picture

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.

Mile23’s picture

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)));
  }

}
?>
YesCT’s picture

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

Mile23’s picture

Issue summary:View changes

Linked to @covers documentation.

YesCT’s picture

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() {
YesCT’s picture

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

ParisLiakos’s picture

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

Mile23’s picture

As far as @return, there's this other discussion happening: #2253915: [policy, then 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.

cordoval’s picture

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

dawehner’s picture

@cordoval
I try to understand what you ask for. Do you want to write some kind of integration test with an already setup container?
Drupal has something called KernelTestBase, see #2259275: Rename "DrupalUnitTestBase" to KernelTestBase which though is not a phpunit framework based test.

cordoval’s picture

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.

dawehner’s picture

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.

Sorry but I still don't get it. The discussion seems to be a little bit out of scope of this issue, tbh.

The KernelTestBase is seen as an integration test.

cordoval’s picture

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?

Mile23’s picture

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

Mile23’s picture

Issue summary:View changes
sun’s picture

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?

sun’s picture

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)

Mile23’s picture

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

I wanted to disagree first but I realized that specifing @group Drupal is indeed pointless. If you want to run just the Drupal related tests you run phpunit in the core folder. The fact that we still use simpletest in the middle to run these tests is just a historic problem.

For other use-cases like run all drupal + contrib tests but no symfony tests I would argue that you should either just accept to run the other tests as well, or execute phpunit in specific folders and merge the results.

+1 to remove this pointless verbosity.

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.

There are components which exists in both Drupal\Core and Drupal\Components namespace, but yeah I think there is no need to distinct the two groups.

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.

Totally agree, if the test is a pure unit test use @coversDefaultClass and @covers for each method.

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

Nice idea!

sun’s picture

Status:Reviewed & tested by the community» Needs review

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.

jhodgdon’s picture

Project:Drupal core» Drupal Technical Working Group
Version:8.0.x-dev»
Component:phpunit» Code

Coding standards changes are now part of the TWG

Mile23’s picture

Issue summary:View changes

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

Mile23’s picture

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?

daffie’s picture

Todo add @depends to the documentation.

daffie’s picture

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

dawehner’s picture

@daffie
Nope, @depends has a an additional semantic meaning ... it runs after another test so it depends.

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

daffie’s picture

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

dawehner’s picture

Tests must pass under --strict. This means, for instance, that all tests must make an assertion.

I would actually vote for: Tests should pass under --strict, but don't have to strictly.

Mile23’s picture

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

jhedstrom’s picture

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

<?php
 
/**
   * @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.

jhodgdon’s picture

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.

ParisLiakos’s picture

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.

Mile23’s picture

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

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

YesCT’s picture

heddn’s picture

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

dawehner’s picture

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

Mh, good to know that this is possible, but I kinda disagree because we also use the test prefixes in all of your simpletest based tests ...