Add @covers annotation, fix some --strict for PHPUnit

1. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheTest.php
   @@ -127,13 +128,13 @@ public function testDeleteTags() {
      public function testInvalidateTags() {
   -    Cache::deleteTags(['node' => [2, 3, 5, 8, 13]]);
   +    Cache::invalidateTags(['node' => [2, 3, 5, 8, 13]]);
   

   ah, this is the "controversial" thing you mentioned.

2. +++ b/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php
   @@ -46,7 +46,7 @@ protected function setUp() {
          'storage_settings' => array(
   -        'some_setting' => 'value 1'
   +        'some_setting' => 'value 1',
   

   this looks out of scope.

the rest of this looks ok to me (but I didn't run the coverage to see the diff in complaints before and after the patch)

I commented on #2340123: Setting cache tags can be tricky: use strings instead of nested arrays to improve DX

Here's another #2366663: ContextHandlerTest misuses @covers

this is a great issue. thanks for filing it. and for working on fixing it.

@tim.plunkett yeah, this patch takes care of that.

I have some questions and feedback (and I documented how I tested/reviewed it).

1
./vendor/phpunit/phpunit/phpunit --filter=ResponsiveImageMappingConfigEntityUnitTest --coverage-html /tmp/report

in head, without this patch:
.Trying to @cover or @use not existing method "\Drupal\responsive_image\Entity\ResponsiveImageMapping::hasMapping".

with the patch:

.......

Time: 55.89 seconds, Memory: 135.00Mb

OK (7 tests, 17 assertions)

2
./vendor/phpunit/phpunit/phpunit --filter=PermissionHandlerTest --coverage-html /tmp/report
before: Trying to @cover or @use not existing method "\Drupal\user\PermissionHandler::buildPermissions".
after: finishes

I dont think there ever was a buildPermissions in PermissionsHandler (there is one in NodePermissions).

3.
./vendor/phpunit/phpunit/phpunit --filter=CacheTest --coverage-html /tmp/report
before: Trying to @cover or @use not existing class or interface "validateTags".
after: finishes

phpstorm complains that a void menthod is having it's result used.
but asserting something seems better than relying on not an exception.

the other changes in here are good (adding :: to before the method name in the @covers line). I also checked the test actually covered it, and they do.

4.
./vendor/phpunit/phpunit/phpunit --filter=DrupalTest --coverage-html /tmp/report
before: Trying to @cover or @use not existing method "::requestStack"
after:

checked this is adding the correct default class.
checked each test, and it does call the method specified by the additional @covers
only the ::l one looked strange, cause it didn't match the oneline summary which said _l() (but I think that summary is wrong, it is called l() )

5.

+++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
@@ -195,6 +224,8 @@ public function testEntityQuery() {
    * Tests the entityQueryAggregate() method.
+   *
+   * @covers ::entityQueryAggregate

we might be able to say in this issue to take out these one line summaries.. *but* the working group https://www.drupal.org/governance/technical-working-group has not actually set the standard for our phpunit docs #2057905: [policy, no patch] Discuss the standards for phpunit based tests, so might as well leave these in.

6.
6a.

+++ b/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php
@@ -46,7 +46,7 @@ protected function setUp() {
-        'some_setting' => 'value 1'
+        'some_setting' => 'value 1',

is out of scope.

6b.

+++ b/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php
@@ -305,7 +307,7 @@ public function testNullDefaultValueCallback() {
-  public static function mockDefaultValueCallback($entity, $definition) {
+  public static function mockDefaultValueCallback(EntityInterface $entity, FieldDefinitionInterface $definition) {

this might be out of scope also.. is it needed to get the --strict to run?

6c.
./vendor/phpunit/phpunit/phpunit --filter=BaseFieldDefinitionTest --coverage-html /tmp/report
before:
(this time it does run)

................

Time: 1.07 minutes, Memory: 130.75Mb

OK (16 tests, 27 assertions)

Generating code coverage report in HTML format ...

after:

................

Time: 1.03 minutes, Memory: 134.75Mb

OK (16 tests, 29 assertions)

Generating code coverage report in HTML format ...

so then I tried running with --strict:
./vendor/phpunit/phpunit/phpunit --filter=BaseFieldDefinitionTest --strict --coverage-html /tmp/report2
before:

RRRRRRRRRRRRRRRR

Time: 1.18 minutes, Memory: 130.75Mb

OK, but incomplete, skipped, or risky tests!
Tests: 16, Assertions: 27, Risky: 16.

Generating code coverage report in HTML format ...

after:

RRRRRRRRRRRRRRRR

Time: 1.62 minutes, Memory: 134.75Mb

OK, but incomplete, skipped, or risky tests!
Tests: 16, Assertions: 29, Risky: 16.

6d. same number of risky tests.
hm. but only 2 more assertions. odd because patch adds asserts (assertNotNull) to 3 tests that did not have any asserts. 3. does something similar. and the results of the code coverage (even with --strict) before and after are the same... so maybe these added assertNotNull() are out of scope also?

7.
fixes @covers
but also adds assertNotNull() to add assert to tests.

./vendor/phpunit/phpunit/phpunit --filter=SqlContentEntityStorageSchemaTest --coverage-html /tmp/report
before: Trying to @cover or @use not existing method "\Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSchema".
after:

8.
./vendor/phpunit/phpunit/phpunit --filter=ContextHandlerTest --coverage-html /tmp/report
before: Trying to @cover or @use not existing method "\Drupal\Core\Plugin\Context\ContextHandler::__construct".
after: finishes

(yeah, and I think setUp() should not @cover anything?)

9.
./vendor/phpunit/phpunit/phpunit --filter=DefaultPluginManagerTest --coverage-html /tmp/report
before: .......Trying to @cover or @use not existing method "\Drupal\Core\Plugin\DefaultPluginManager::enforcePluginInterface".
after: finishes

ok. removes a wrong @covers.
but also adds asserts for two tests that did not have asserts.

10.
./vendor/phpunit/phpunit/phpunit --filter=AccessAwareRouterTest --coverage-html /tmp/report
before ..Trying to @cover or @use not existing method "\Drupal\Core\Routing\Router::__call".
after finishes

this makes sense here, the change in the default covers class

11.
./vendor/phpunit/phpunit/phpunit --filter=ReverseProxyMiddlewareTest --coverage-html /tmp/report
before: OK (4 tests, 6 assertions)
after: OK (3 tests, 6 assertions)

yeah, the naming of the data provider method should not start with test.
and that is why there is 1 less test (and the same number of assertions, cause there are no assertions in a provider)
(but I dont know if --strict caught that for you...)

trying
./vendor/phpunit/phpunit/phpunit --filter=ReverseProxyMiddlewareTest --strict --coverage-html /tmp/report
before:

...R

Time: 1.35 minutes, Memory: 130.50Mb

OK, but incomplete, skipped, or risky tests!
Tests: 4, Assertions: 6, Risky: 1.

after:

...

Time: 1.4 minutes, Memory: 134.25Mb

OK (3 tests, 6 assertions)

yeah, this time I can see a change in the risky test numbers using --strict, so this looks in scope.

I dont see how the changes 6c
effect the output of
./vendor/phpunit/phpunit/phpunit --filter=BaseFieldDefinitionTest --strict --coverage-html /tmp/report2

it passes strict before (I think the OK means it passes), and the results of the number of risky tests are the same: 16.
Is there something else we need to do here to make them not risky?
Or are the asserts part of moving them toward not being risky?

To jump in on #19; I'd love to see #2358657: Wrong @covers definitions in Drupal project go in soon so my automated tests/coverage report graphs can have joy and happiness again—they have been empty for some time now :(

#2358657: Wrong @covers definitions in Drupal project is fixed, reopening this issue.

Also, is #2287385: Fix PHPUnit coverage tests related to this issue?

1. +++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
   @@ -228,6 +262,8 @@ public function testModuleHandler() {
       * Tests the typedData() method.
   +   *
   +   * @covers ::typedDataManager
   
   @@ -277,8 +320,9 @@ public function testLinkGenerator() {
   -   * Tests the _l() method.
   +   * Tests the l() method.
       *
   +   * @covers ::l
   

   similar to the rationale for fixing the _l() comment, we should fix this since there is no typedData() method.

2. +++ b/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php
   +++ b/core/tests/Drupal/Tests/Core/Entity/BaseFieldDefinitionTest.php
   @@ -295,7 +295,8 @@ public function testCustomStorage() {
   
   +++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
   @@ -276,7 +276,7 @@ public function testGetDefinitionsWithoutRequiredInterface() {
   -    $plugin_manager->getDefinitions();
   +    $this->assertInternalType('array', $plugin_manager->getDefinitions());
   

   I looked at
   https://phpunit.de/manual/current/en/appendixes.assertions.html#appendix...

   to check if it fails if it gets NULL, but it didn't say there.

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
@@ -898,7 +904,9 @@ public function testDedicatedTableSchema() {
+      $this->storageSchema->onFieldStorageDefinitionCreate($field_storage)

should we add a @covers for onFieldStorageDefinitionCreate ?

ok. updating the issue summary to reflect what I understand about this issue.

Committed c1b037d and pushed to 8.0.x. Thanks!