Remove () from a bunch of @covers definitions in PHPUnit

k

Please ensure that this is covered in the phpunit standard issue!

Sorry, but I think because we require parens with @see it would be more consistent to require them here as well. Let's at least get some more input on this.

Coding standards changes are now part of the TWG

Nope. #6 is incorrect (we also don't follow symphony cs) and also no reference has been linked here.

OK, so the link in the IS is not really a coding standard per sé, but sure, if all the examples follow a certain pattern that can be considered a standard. You're right that if there is no reason not to, then following the standard is a good thing. I still think that consistency with @see would be one reason not to, but surely weighing the two is a matter of personal opinion.

That said, and in case I didn't make that clear above, I have absolutely no problem with this patch going in as is, I just don't see any reason to rush this at all and I don't think there is something that can be called a consensus here.

So how about we just leave this at "needs review" and see what some other folks think about this?

I would RTBC this if it applied.

It would require heroic effort to re-roll, given the number of piecemeal @covers changes.

PHPUnit conventions are plenty fine for Drupal.

Here's a re-roll.

There are still scads of () out there, which you can find with this regex: @covers ::\w+\(

I'll poke at them over the next few days.

Removed ExternalUrlTest.php since it was removed in #2343759: Provide an API function to replace url()/l() for external urls.

Previous patch adds out-of-scope stuff to settings and services files, possibly due to poor re-roll. Fixed.

Previos patch adds testFieldSqlSchemaForEntityWithStringIdentifier() back, definitely due to poor re-roll. :-) Fixed.

I think I'll leave this patch for whoever wants to RTBC. Other () fixes will be in related patches.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityResolverManagerTest.php
@@ -69,7 +69,7 @@ protected function setUp() {
-   * @covers ::getControllerClass
+   * @covers ::getController

note there are some fixes (not just paren removal)

So reviewers may find applying the patch and using git diff --color-words helpful in spotting those.

---

@Mile23

Other () fixes will be in related patches.

Please link those issues, so we know where other () removals will be done.

---

Also, since this is a normal task, we should update the summary with an "Allowed in beta?" section,
with info from https://www.drupal.org/core/beta-changes

1. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php
   @@ -80,18 +80,20 @@ protected function setUp() {
   -   * @covers ::getEntityIndexName()
   -   * @covers ::getFieldIndexes()
   ...
   +   * @covers ::getEntityIndexName
   +   * @covers ::addFieldSchema
   +   * @covers ::getFieldIndexes
   
   @@ -485,12 +488,13 @@ public function testGetSchemaRevisionable() {
   @@ -574,13 +578,14 @@ public function testGetSchemaTranslatable() {
   
   @@ -574,13 +578,14 @@ public function testGetSchemaTranslatable() {
   -   * @covers ::__construct()
   -   * @covers ::getEntitySchemaTables()
   -   * @covers ::initializeDataTable()
   ...
   +   * @covers ::__construct
   +   * @covers ::getSchema
   +   * @covers ::getEntitySchemaTables
   

   why are we adding getSchema here?

   A separate issue for that seems reasonable.

   Similar for addFieldSchema

2. +++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php
   @@ -223,7 +223,7 @@ public function testGetDataTable() {
       * @cover ::__construct()
   

   we should fix these here... if there is not a separate issue fixing the @cover -> @covers (or even if there is... it should be easy to reroll for this)

3. +++ b/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php
   @@ -45,7 +47,9 @@ public function testGetName() {
   +   * Tests langcode ID getter and setter methods.
   +   *
   

   I dont think we should fix the one line summary coding standards thing in this issue. Especially, since we may not do that for phpunit tests.

4. +++ b/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php
   @@ -54,7 +58,10 @@ public function testGetLangcode() {
   +   * @covers ::getDirection
   +   * @covers ::setDirection
   

   setDirection is no longer a method.

taking out the
- * @covers ::getControllerClass
+ * @covers ::getController

change.

I think that makes these changes only in scope now.

Adding beta phase evaluation stuff.

oops. files...

regarding #17 1.
#2358657: Wrong @covers definitions in Drupal project took out the getSchema and addFieldSchema, so we should not add them back in. I think it was just an artifact of the reroll.

and #2300131: EntityResolverManager instantiates objects unnecessarily renamed getController to getControllerClass, so let's leave it getControllerClass in the covers also. ;)

So I dont think we need any separate issues.

Yes, there were far fewer than I thought. I guess I should have done the search on the *patched* version eh?

Anyway, here's the new hotness, removing all the other parentheses I could find, which passes ./vendor/bin/phpunit --strict --coverage-html. This strict coverage mode complains about erroneous @covers, so I think we're good.

Updated #2057905: [policy, no patch] Discuss the standards for phpunit based tests with info about not using parentheses.

reviewed that. looks super good to me, but I think I did too much to rtbc it.

I went through this patch carefully with color-words and have verified that the only changes are @cover -> @covers and removing () at the end of those lines. This is good to go. Soon so it does not need reroll.

Tested with --strict --coverage-text, and it works great (1 fail, but that's due to #2373549: PHPUnit test testGetDoesntHitConsistentBackend failing when run with coverage reporting, which has an RTBC patch :). I like following PHPUnit's conventions here, and agree with IS.

git ac https://www.drupal.org/files/issues/phpunit-2328919-23.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 92351  100 92351    0     0  74738      0  0:00:01  0:00:01 --:--:-- 83803
error: patch failed: core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php:80
error: core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageSchemaTest.php: patch does not apply

Rebase of #23.

Back to RTBC.

This issue is a unfrozen change (documenation and tests) as per https://www.drupal.org/core/beta-changes. Committed 1a73d7e and pushed to 8.0.x. Thanks!