Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 bySuperCoreInjectableTest
. - 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 withprovider
. For example:testSuperCoreCanInject()
would have a data provider ofproviderSuperCoreCanInject()
. This is a guideline, and it may make more sense to have a readable method name rather than one that starts withprovider*
. 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()
andtestFrammistatForException()
, 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
Comment #1
dawehnerSome ideas for the standard i have in mind:
Comment #2
msonnabaum CreditAttribution: msonnabaum commentedI mostly agree with these.
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.
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.
I don't quite understand this.
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.
Comment #3
dawehnerYeah we could add them as well. For example all the routing components of core should be tested like that.
Yeah for things like the request or the DIC I hate to mock them, as it needs quite some time.
On simpletest we used to avoid test functions, as this really cost time. This is not really the case anymore with phpunit.
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.
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedComment #5
Mile23You 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.
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
Comment #6
dawehnerThis is awesome! It would be great to have both full drupal as well as based upon component as you suggested.
Oh I thought every function has to start with "test" and not end with?
Comment #7
Mile23Ugh. yah. :-)
testControllerCanReturnJson() :-)
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commented#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
Comment #9
jhedstromTurning 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.
Comment #10
dawehnerWell, 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?
Comment #11
Mile23Editing the issue summary now...
Comment #12
Mile23OK, I summarized the discussion so far and put it in the issue summary.
Is it time to add the documentation tag?
Comment #13
dawehnerI 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.
Comment #14
Mile23It'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. :-)
Comment #15
Mile23More voices please. :-)
Comment #16
Mile23We 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?
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedi 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
Comment #18
tim.plunkett#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.
Comment #18.0
Mile23Updated with actual stuff at comment #11.
Comment #19
Mile23So I'll take that as a yes, tests should pass with --strict, which generally means having to throw an assertion.
Updated the issue summary.
Comment #19.0
Mile23Added --strict
Comment #20
Mile23Comment #21
msonnabaum CreditAttribution: msonnabaum commentedPlease, 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.
Comment #22
dawehner+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.
Comment #23
kim.pepperI'd prefer 'Fake' instead of Sub_. Makes more sense IMHO.
Comment #24
Mile23'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.
Comment #25
kim.pepperMartin Fowler gives his definitions here: http://martinfowler.com/articles/mocksArentStubs.html
Comment #26
YesCT CreditAttribution: YesCT commentedThere 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
Also, tests use a lot of chaining so #1516460: Define coding standard for chained method calls might be related.
Comment #27
YesCT CreditAttribution: YesCT commented@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)
---------
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
Comment #28
Mile23@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.)
Comment #29
dawehnerThis is simply pointless, as this won't catch 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.
Comment #30
Mile23Well, 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?
Comment #31
tim.plunkettYes, 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.
Comment #32
dawehnerYeah 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.
Comment #33
Mile23@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.
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:
Comment #34
YesCT CreditAttribution: YesCT commentedI 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
Comment #35
Mile23Linked to @covers documentation.
Comment #36
YesCT CreditAttribution: YesCT commentedShall 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.
b.
c.
Comment #37
YesCT CreditAttribution: YesCT commentedIn the summary, it says:
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().
Comment #38
ParisLiakos CreditAttribution: ParisLiakos commentedMaybe even
provideTestSuperCoreCanInject
, which also solves #21 and is nice to read :) but maybe too verboseI 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
Comment #39
Mile23As 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.
Comment #40
cordoval CreditAttribution: cordoval commentedpeople 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
Comment #41
dawehner@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.
Comment #42
cordoval CreditAttribution: cordoval commentedno, 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.
Comment #43
dawehnerSorry 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.
Comment #44
cordoval CreditAttribution: cordoval commentedI 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?
Comment #45
Mile23@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. :-)
Comment #46
Mile23Comment #47
sunReading the latest revision in the issue summary, I'd like to raise some concerns + suggestions on the Documentation chapter:
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.
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.
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?
Comment #48
sunIn 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)
Comment #49
Mile23#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?"
An integration test isn't a unit test, so it should be marked @coversNothing.
Comment #50
dawehnerI 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.
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.
Totally agree, if the test is a pure unit test use @coversDefaultClass and @covers for each method.
Nice idea!
Comment #51
sunCool. 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.
Comment #52
jhodgdonCoding standards changes are now part of the TWG
Comment #53
Mile23In light of #2328919: Remove () from a bunch of @covers definitions in PHPUnit I added a bit about not using parentheses in @covers.
Comment #54
YesCT CreditAttribution: YesCT commentedShould we have @uses ?
See #2371531-10: Expand unit testing for Drupal\Component\Plugin\PluginManagerBase
https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi...
Comment #55
Mile23If 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:
This means there's a discrepancy between whether, for instance,
\Drupal
is covered by this test. That's becauseUnitTestCase::setUp()
always uses\Drupal
to set the container toNULL
, 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?
Comment #56
daffie CreditAttribution: daffie commentedTodo add @depends to the documentation.
Comment #57
daffie CreditAttribution: daffie commentedRemove @see. We have @coversDefaultClass and @covers for this.
Comment #58
dawehner@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.
Comment #59
daffie CreditAttribution: daffie commented@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.
Comment #60
dawehnerI would actually vote for: Tests should pass under --strict, but don't have to strictly.
Comment #61
Mile23Not 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.
Here's what I would write: "Avoid @depends. It reduces isolation between tests."
Comment #62
jhedstromWhat 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
The documentation page doesn't mention assert messages.
Comment #63
jhodgdonI 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.
Comment #64
ParisLiakos CreditAttribution: ParisLiakos commentedwe 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.
Comment #65
Mile23Just a heads-up about this... #2415441: Automate finding @covers errors
It's both a documentation issue and a code-quality one.
Comment #66
YesCT CreditAttribution: YesCT commented#2462037: [policy, no patch] Discuss usage of @codeCoverageIgnore seems related
Comment #67
heddnShould we be using @test and not prefixing the test with the name test?
Comment #68
dawehnerMh, 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 ...
Comment #69
tizzo CreditAttribution: tizzo commentedMoving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.
Comment #70
alexpottSome 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:
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.
Comment #71
alexpottNow 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.
Comment #72
jhodgdonWe should at least put a note at the top of it that it's only for Drupal 6/7?
Comment #73
alexpott@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:
Because our coding standards in Drupal have long since removed the @file docblock.
Comment #74
donquixote CreditAttribution: donquixote commented@ParisLiakos (#64)
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.