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

Various base classes are meant to be abstract, but were not declared correctly. That is only possible because our current (too error-forgiving) test discovery process silently ignores the errors.

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

CommentFileSizeAuthor
#7 test.base-abstract.7.patch11.3 KBsun
test.base-abstract.0.patch8.39 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, test.base-abstract.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

test.base-abstract.0.patch queued for re-testing.

jhodgdon’s picture

The doc blocks for several of these supposed base classes does not agree with them being abstract base classes. For instance:

 /**
  * Tests for update translations.
  */
-class LocaleUpdateBase extends WebTestBase {
+abstract class LocaleUpdateBase extends WebTestBase {

The docs say it is actually doing some tests. ?!? Actually several of the classes do not even have doc blocks. :(

jhodgdon’s picture

Oh I see that you did the doc blocks on a separate issue
#2262195: Various test classes are missing phpDoc

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

Would it be too much to ask to update the *existing* doc blocks on the classes you are making abstract here? The empty ones are being taken care of on the other issue, but several of the existing ones are still out of synch with their being abstract base classes.

Also I think we have a naming standard that classes meant to be used as base classes should have names ending in "Base". Could that also be fixed, like here:

++ b/core/modules/options/lib/Drupal/options/Tests/OptionsDynamicValuesTest.php
@@ -12,7 +12,7 @@
 /**
  * Sets up a Options field for testing allowed values functions.
  */
-class OptionsDynamicValuesTest extends FieldTestBase {
+abstract class OptionsDynamicValuesTest extends FieldTestBase {

... See... Is this really a test base class at all? You've made it abstract but it's not obvious from the name of the class or its documentation that it should be?

Maybe what I'm asking for is scope creep in this issue, but if it is, can we have a follow-up to fix it?

sun’s picture

FileSize
11.3 KB

Technically that is out of scope for this issue, yes.

Those base classes are meant to be abstract already, just weren't declared correctly. That is only possible because our current (too error-forgiving) test discovery process silently ignores the error. The parent issue will prevent this from happening again in the future.

But OK, I'm happy to perform those additional adjustments — but only as long as we will quickly move forward here. If they cause this trivial patch to get delayed, then I will revert them.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! This looks good to me. +1 for making the names better and making sure all the docs say "Base class for something". Seems logical that they should all be abstract if they're not actually testing something themselves or meant to be run as tests.

sun’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3d31d12 and pushed to 8.x. Thanks!

  • Commit 3d31d12 on 8.x by alexpott:
    Issue #2262147 by sun: Various base test classes are not abstract.
    

Status: Fixed » Closed (fixed)

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

sun’s picture