The Drupal\Tests\Core\Plugin\Discovery\ContainerDerivativeDiscoveryDecoratorTest misses a @covers and a @coversDefaultClass documentation.

The documentation for the phpunit based test can be found in #2057905: [policy, no patch] Discuss the standards for phpunit based tests.

CommentFileSizeAuthor
#5 interdiff-2409647-2-5.txt929 byteszealfire
#5 revised-container-derivative-discovery-test-2409647-5.patch945 byteszealfire
#2 revised-container-derivative-discovery-test-2409647-2.patch1.03 KBzealfire
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie’s picture

daffie CreditAttribution: daffie commented
Issue summary: View changes
zealfire’s picture

zealfire CreditAttribution: zealfire commented
Assigned: Unassigned » zealfire
FileSize
revised-container-derivative-discovery-test-2409647-2.patch1.03 KB

I have tried to make a patch after reading the docs of @covers but not sure.Please review.
Thanks

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Active » Needs review

So that the testbot runs.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/ContainerDerivativeDiscoveryDecoratorTest.php
@@ -13,6 +13,7 @@
  * Tests the container aware derivative discovery decorator.
  *

@@ -20,6 +21,8 @@ class ContainerDerivativeDiscoveryDecoratorTest extends UnitTestCase {
    * Tests the getDerivativeFetcher method.
    *

These lines can be removed. The lines that you add say the same. :-)

zealfire’s picture

zealfire CreditAttribution: zealfire commented
Status: Needs work » Needs review
FileSize
revised-container-derivative-discovery-test-2409647-5.patch945 bytes
interdiff-2409647-2-5.txt929 bytes

Implemented the missed things in new patch .Please review.
Thanks.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

Good work zealfire.

It all looks good to me.
It is all documentation, so it is allowed. See beta-changes.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 369771b and pushed to 8.0.x. Thanks!

  • alexpott committed 369771b on 8.0.x 
    Issue #2409647 by zealfire: ContainerDerivativeDiscoveryDecoratorTest...
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Title: ContainerDerivativeDiscoveryDecoratorTest misses @covers and coversDefaultClass documentation » Figure out if ContainerDerivativeDiscoveryDecoratorTest actually covers getDerivativeFetcher()
Priority: Minor » Normal
Status: Fixed » Needs work

There's no such thing as ContainerDerivativeDiscoveryDecorator::getDerivativeFetcher(), so this test cannot cover it. Trying to generate a coverage report results in an error.

If the testbot were to generate a coverage report automatically, this sort of error would not happen, either in the first place or in subsequent fixes.

Looking at the code of the test, we see that it asserts against the result of ContainerDerivativeDiscoveryDecorator::getDefinitions(), which would (erroneously) suggest that a proper @covers annotation might be for getDefinitions().

    $discovery = new ContainerDerivativeDiscoveryDecorator($discovery_main);
    $definitions = $discovery->getDefinitions();

    // Ensure that both the instances from container and non-container test derivatives got added.
    $this->assertEquals(4, count($definitions));

However, ContainerDerivativeDiscoveryDecorator doesn't actually implement getDefinitions() (its parent does), so if this test is trying to cover any of ContainerDerivativeDiscoveryDecorator's methods, it's either a poor test, or uses side-effects in some way not clear from the test.

In fact, ContainerDerivativeDiscoveryDecorator only has one method: getDeriver(), which doesn't appear to have any coverage at all. I can't tell for sure because I'm unable to generate a coverage report because of this error. :-)

So, next steps:

  • Figure out what this test *does* cover, and annotate it and/or move it to the namespace for the class it tests.
  • Write a real unit test for ContainerDerivativeDiscoveryDecorator::getDeriver(), probably in another issue.
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue tags: -Novice

The further tasks are not novice.

  • alexpott committed 369771b on 8.1.x 
    Issue #2409647 by zealfire: ContainerDerivativeDiscoveryDecoratorTest...
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Fixed
Related issues: +#2620330: Move ContainerDerivativeDiscoveryDecoratorTest::testGetDefinitions() to DerivativeDiscoveryDecoratorTest.

Added #2620330: Move ContainerDerivativeDiscoveryDecoratorTest::testGetDefinitions() to DerivativeDiscoveryDecoratorTest.

Re-closing this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.