Blocks #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)

This patch fixes all test classes that are missing phpDoc.

The parent issue will prevent this from happening in the future.

CommentFileSizeAuthor
#12 test.phpdoc.12.patch3.56 KBsun
#10 test.phpdoc.10.patch3.55 KBsun
#4 interdiff.txt1.76 KBsun
#4 test.phpDoc.4.patch22.64 KBsun
#2 interdiff.txt3.39 KBsun
#2 test.phpDoc.2.patch22.64 KBsun
test.phpDoc.0.patch21.01 KBsun

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

The first few classes in the patch have doc blocks that do not agree with our standards:
- Should start with a verb
- Verb should be 3rd person, like "tests"
- Should be one line and end in sentence, less than 80 characters.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new test.phpDoc.2.patch22.64 KB
new interdiff.txt3.39 KB

Fair enough. Fixed those instances.

At least for test classes, we'll soon change our standard for phpDoc summary lines to allow to wrap onto the next line (following PSR-5), but that's for another issue.

This patch obeys the current rule of just one line, not exceeding 80 chars.

Also discovered two more instances in PHPUnit tests after fully completing the new test discovery implementation. — Please note that plenty of our PHPUnit tests are only using @coversDefaultClass in their phpDoc already, and this patch follows that convention accordingly. The new test discovery automatically converts that information into a "Tests $classname." description in the UI listing.

I hope we can get this fix quickly out of the way, because all of these changes are making the new test discovery patch borderline impossible to review (which is why I did not post it yet).

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Looks good! Coding standards are OK now. A few typos:

a)

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateSystemPerformanceTest.php
@@ -11,6 +11,9 @@
 use Drupal\migrate\MigrateExecutable;
 use Drupal\migrate_drupal\Tests\MigrateDrupalTestBase;
 
+/**
+ * Tests migration of system performance variable to configuration.
+ */
 class MigrateSystemPerformanceTest extends MigrateDrupalTestBase {

(variable => variables)

b) Capitalization is a bit weird here, is this right?

+++ b/core/modules/system/lib/Drupal/system/Tests/System/AdminMetaTagTest.php
@@ -9,6 +9,9 @@
 
 use Drupal\simpletest\WebTestBase;
 
+/**
+ * Tests the fingerprinting Generator HTML meta tag.
+ */
 class AdminMetaTagTest extends WebTestBase {

c) Weird caps here too:

+/**
+ * Tests 404 Not found pages and custom 404 pages.
+ */
 class PageNotFoundTest extends WebTestBase {

And I'm not sure about committing this quickly... it touches a lot of files. Could be considered "disruptive"?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new test.phpDoc.4.patch22.64 KB
new interdiff.txt1.76 KB

Fixed all three points.

Regarding the question of possibly being disruptive: Fair concern. That said, the file headers of existing test classes are usually not touched by other patches (as opposed to the header of files containing functional code), so I'd tentatively say that it shouldn't cause any negative effects on other patches in the queue.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good! I'll let one of the other maintainers commit this, as they can make a better decision on "is this disruptive".

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue summary: View changes
alexpott’s picture

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

Committed 8494b06 and pushed to 8.x. Thanks!

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/MailCaptureTest.php
@@ -9,6 +9,9 @@
diff --git a/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php

diff --git a/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php
index 83f1048..e46051d 100755

index 83f1048..e46051d 100755
--- a/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php

--- a/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php

Changed the file mode back to 644 during commit :)

  • Commit 8494b06 on 8.x by alexpott: 
    Issue #2262195 by sun: Various test classes are missing phpDoc.

Status: Fixed » Closed (fixed)

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

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Closed (fixed) » Needs review
StatusFileSize
new test.phpdoc.10.patch3.55 KB

Quick follow-up patch for new instances that were introduced since this patch was committed.

The parent issue will prevent this from happening again in the future.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Could we fix the grammar in this one?

+/**
+ * Tests LocaleLookup does not cause circular references.
+ */

Should have "that" as 2nd word.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new test.phpdoc.12.patch3.56 KB

Sure

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

  • alexpott committed d7efdaa on 8.x 
    Issue #2262195 by sun: Fixed Various test classes are missing phpDoc.
alexpott’s picture

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

Committed d7efdaa and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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