Add return typehints to setUp/tearDown methods in concrete test classes

Attached patch created with this script, which adds the return type for everything that isn't related to legacy SimpleTests, plus one or two manual fixes on top.

find core -name '*.php' | grep -v simpletest/src | xargs -n1 grep -EL 'use.*simpletest' | xargs sed -E -i "s/(function (setUp|tearDown|setUpBeforeClass|assertPostConditions)\(.*\)) /\1: void /"

Missed one class that mentions simpletest and four that use "setup()" instead of "setUp()". Some method visibilities are also wrong but I think that is out of scope to fix here.

If this isn't viable because of the BC break with contrib/custom tests then I think this is still worth doing on the subclasses. We could emit a deprecation warning by using ReflectionMethod::hasReturnType() to tell contrib/custom code that they need to add the return type.

Thanks, fixed those. The ones further down the class hierarchy are always going to end up false positives when using basic tools like find/grep/sed.

This needs a CR and a release note. Thanks!

I agree that this is a big issue to solve with PHPUnit 8.

The downside of this solution is that if you have a module where you want to support 8.8 and 9.0 with the same codebase you minimum PHP is then 7.1. I think this is pretty unavoidable. The only other solution is to move to Symfony's simple-phpunit and that was not at all simple and will be even more disruptive given we'd have to drop PHPUnit as dev dependency of core. Also PHP 7.0 is not longer supported so I think the trade-off here is okay given what we have to work with.

So I'm +1 to the idea but the issue summary needs a major update to spell out the impacts of this change on the ecosystem.

Leaving "needs framework manager review" on the issue for other framework managers to have their say.

I think we should still be doing this in 9.0.x so that we're not relying on our own bc layer. We could potentially delay any base test classes to 9.1.x though but the regular tests should be brought up to date.

Even when excluding the base classes that might be tricky and will break at least a few modules. redis for example re-uses a few tests from core (lock and queue) and with complex hierarchies like the rest/jsonapi test structure, what exactly is a base class, what do we change and what not?

I'm not saying that's a definitive blocker, redis for example has been broken a few times with those tricks as we don't really support that anyway, although also in good ways (extended coverage in core showing of bugs in the redis implementation). Just something to consider when doing it. and unless we add it to our base classes, we 100% rely on our BC layer to anyway, the final breaking patch would just be smaller.

I think something like redis should expect to update its test coverage every so often (although that's a reason not to mark tests final, since I think it's a valid use-case to piggyback off core tests like that and people know what they're doing when they do it).

and unless we add it to our base classes, we 100% rely on our BC layer to anyway

This is true but it'd be a lot less bad examples in core for people to potentially copy/paste from.

Nice work on the deprecation test :)

I think as per @alexpott in #3063887-75: Support PHPUnit 8 in Drupal 9, drop support for PHPUnit 7 we need to only add the typehints here in 9.0.x. Do you want to spin off the deprecation into a new issue for 9.1.x?

+++ b/core/tests/Drupal/Tests/Listeners/DrupalListener.php
@@ -18,11 +18,41 @@ class DrupalListener implements TestListener {
+          if (!$reflected_method->getReturnType() || $reflected_method->getReturnType()->getName() !== 'void') {

Real tiny nitpick but you can say ->hasReturnType() in the first half of the if statement here.

Nothing more to do here as far as I can see, marking RTBC to get core committers attention on this proof of concept and help figure out next steps to get this committed.

New tests introduced, they have to be updated appropriately.

Added draft change record: https://www.drupal.org/node/3114724

Updated issue summary and added a draft line for the release notes, but perhaps it could be worded better.

I rerolled the patch on #50.
It didn't apply so i worked on the conflicts shown by git. After that, the [git rebase --continue] was successfull, but the following step failed.
[Testing my patch by applying it to the upstream branch]. See the attached txt for more info.

Thanks @salah1!

Fixed up the patch to include new test methods and a merge error. Also updated the deprecation message to link to the CR.

I am working on this

Fixed some test fail in comment #57. Kindly review new patch.

Reroll of #60 due to conflict in core/modules/file/tests/src/Kernel/Migrate/d7/MigratePrivateFileTest.php

Removed deleted patch code from file and updated with some test fix,

As this affects almost every test class and has needed repeated rerolls I wonder if this is going to need a fixed window to try and get this in, similar to #2822382: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase - it seems futile rerolling it when almost every core commit breaks it again.

I am +1 on scheduling this, but I am not sure when for yet - agreed it's the sort of patch where a specific commit window would be beneficial.

@xjm was not sure about committing this to 9.x at all, I think it's worth it:

the current bc layer we added rewrites test classes, but this is forward compatible with tests that implement the return type hints.

Doing this patch is equivalent to removing deprecated usages for a 'normal' deprecation - then in 10.x we would only need to add the type hints for the base classes (a test API change for everything extending those classes) and remove the bc layer (same but for every conveivable test).

Applying this patch earlier makes it more likely that contrib modules will follow core's example and add the return type hints themselves, something they would ideally do before we remove the bc layer in 10.x.

Updated test class setup().

Remove wrong file, kindly review new patch with interdiff

updated patch #78 for class NodeClassicTest

@catch and I discussed doing this as a scheduled beta target late in the beta process. It should happen before RC (which begins the first week of May) because it could theoretically be disruptive for contrib that extends an individual core test (which some contrib does do). Since individual tests are internal, it's OK to make the change during beta, but we should not do it during RC to avoid disruption.

I'd suggest picking a target few days in late April to roll, review, and commit the change?

@mondranke It's up to the participants to pick the date range because they're the ones committing to do the patch during that timeframe.

• I'd suggest a couple days in the third week of April maybe? (To avoid colliding with any high-priority issues that might be crammed to land before the commit deadline for RC1.)
• A weekday is best for highest committer availability.

We'll want to get the signoffs from the change before that. Waiting to discuss with @catch a little further to confirm RM signoff, and I'll also link it in committer Slack asking for FM signoff.

As a framework manager I sign off on the approach of

• adding typehints to all the concrete test classes in Drupal 9.0.x
• Adding the deprecation in the follow-up #3111044: Deprecate setUp/tearDown concrete test methods that do not specify void return type in 9.1.x
• Trying to maintain the BC layer for as long as possible to make supporting D8 and D9 from the same codebase as simple as possible

@catch and I agreed that this is a good approach for addressing this compatibility issue so that we're ready by Drupal 10, so long as we're also sure to do #3111044: Deprecate setUp/tearDown concrete test methods that do not specify void return type after 9.1.x is opened for development next week. Thanks!

@mondrake that makes sense, although we should still keep this issue against the lowest applicable version.

I may be out of the loop but what "thingie"?

@xjm #3111044: Deprecate setUp/tearDown concrete test methods that do not specify void return type is proposing to add some way to trigger a deprecation notice when a test class doesn't have return type hints - as a new deprecation this could land in 9.1.x-only - this is the 'thingie'. The proposal is to merge that issue back into this one, but with separate 9.1.x and 9.0.x patches (the 9.0.x patch just would not have the hunk adding the deprecation detection/triggering logic).

As this is currently green let's mark it RTBC and get it tested nightly to automatically catch any new tests in the next few days that will need fixing here.

I would suggest adding a sniffer/phpcs rule for this. But to do it in a separate issue as it depends on a new patch and a new release of the coder module. It's good to have the patch(es) here committed first, then the next patch would be much smaller and easy to review, or the current window will be missed probably.

@jungle there does not need to be a PHPCS rule for this. We’re triggering a deprecated error and once we remove the BC layer tests will just break with a language level error.

@alexpott, you are RIGHT! Thanks for your reply!

Committed 0dc2853 and pushed to 9.1.x. Thanks!

Had to edit the diffs to fix core/modules/ban/tests/src/Kernel/Migrate/d7/MigrateBlockedIPsTest.php to core/modules/ban/tests/src/Kernel/Migrate/d7/MigrateBlockedIpsTest.php caused by #3126784: \Drupal\ban\Plugin\migrate\destination\BlockedIP is not identical with its filename BlockedIp.php. Definitely worth getting this done now to prevent re-re-re-re...rolling.

:D lol at the commit message.

FIY, this also changed MigrateProcessTestCase, despite not having a Base suffix, this was a base class that also had subclasses in paragraphs and maybe other contrib modules :)

Yes, this is also effecting migrate_plus and other migrate contrib modules. See https://www.drupal.org/pift-ci-job/1662818

Opened #3131402: Remove return type hints from abstract test classes to deal with that specific case. Also crafted a regex which has discovered some others we may want to consider reverting - some of these are false positives as there are test implementations in the same file, but a few may warrant reverting still:

$ grep -rl 'setUp(): void' core|xargs grep ^abstract
core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php:abstract class SqlContentEntityStorageTestEntityInterface implements EntityInterface {
core/tests/Drupal/Tests/Core/Entity/EntityFieldManagerTest.php:abstract class EntityTypeManagerTestEntity implements \Iterator, ContentEntityInterface {
core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php:abstract class TestEntityHandlerBase extends EntityHandlerBase {
core/tests/Drupal/Tests/Component/PhpStorage/MTimeProtectedFileStorageBase.php:abstract class MTimeProtectedFileStorageBase extends PhpStorageTestBase {
core/modules/migrate/tests/src/Unit/process/MigrationLookupTestCase.php:abstract class MigrationLookupTestCase extends MigrateProcessTestCase {
core/modules/tour/tests/src/Functional/TourTestBasic.php:abstract class TourTestBasic extends TourTestBase {
core/modules/locale/tests/src/Functional/LocaleUpdateBase.php:abstract class LocaleUpdateBase extends BrowserTestBase {
core/modules/media/tests/src/FunctionalJavascript/MediaSourceTestBase.php:abstract class MediaSourceTestBase extends MediaJavascriptTestBase {