Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#7 | test.base-abstract.7.patch | 11.3 KB | sun |
test.base-abstract.0.patch | 8.39 KB | sun | |
Comments
Comment #2
suntest.base-abstract.0.patch queued for re-testing.
Comment #3
jhodgdonThe doc blocks for several of these supposed base classes does not agree with them being abstract base classes. For instance:
The docs say it is actually doing some tests. ?!? Actually several of the classes do not even have doc blocks. :(
Comment #4
jhodgdonOh I see that you did the doc blocks on a separate issue
#2262195: Various test classes are missing phpDoc
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedComment #6
jhodgdonWould 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:
... 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?
Comment #7
sunTechnically 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.
Comment #8
jhodgdonThanks! 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.
Comment #9
sunComment #10
alexpottCommitted 3d31d12 and pushed to 8.x. Thanks!
Comment #13
sunFollow-up: #2293807: Various migrate_drupal unit base test classes are not abstract and implement getInfo()