Support PHPUnit 8 in Drupal 9, drop support for PHPUnit 7

This patch is buidling on the latest #2950132: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5 patch to extend support to PHPUnit 8, implementing option 2 from the IS. Just focusing on Unit tests to keep the patch (relatively :)) smallish. PHPunit's methods are implemented in the compatibility traits, and their test class version-independent equivalents renamed to xxxMethod.

So, here's a new start.

The proposal is:

1. A new Drupal\Tests\BaseTestClass, extending from PHPUnit\Framework\TestCase, is the new base class for ANY Drupal test, being Unit, Kernel, Functional, etc
2. The Drupal\Tests\BaseTestClass is actually aliased to a Drupal\TestTools\PhpUnitCompatibility\PhpUnit#\BaseTestCase class, one for each PHPUnit version, using the compatibility layer mechanism put in place to support PHPUnit 7. These aliased classes hold the override of the ::setUp(), ::tearDown(), etc methods, type hinted according to the specific PHPUnit version.
3. Each overridden method calls a corresponding proxy in the test class so ::setUp() -> ::xxxSetup(). The naming tbd.
4. ALL test classes in Drupal have to rename their PHPUnit overriding methods to the proxies, and any call to parent::method() to the parent proxy, too i.e. parent::setUp() --> parent::xxSetUp()

The patch here is introducing the thing and converting Unit and Build tests, limiting testing to these test types only in DrupalCI. The size is huge just because it's an all or nothing thing... it's tedious but not complicated.
The patch is also lowering the PHPUnit upgrade from PHP 7.3 to PHP 7.1 so we can test with PHPUnit 6, 7 and 8.

Open problems:

• ExpectDeprecationTrait - its version in HEAD is not compatible with PHPUnit 8 since PHPUnit 8 has a ::expectDeprecation() method with a different signature than the one in the trait; also, looks like the overall logic underlying is different.
• MigrateExecutableMemoryExceededTest - it has mock expectation fails in PHPUnit8 due to inconsistent matches(). Added a @todo and commented out the failing tests.
• ViewExecutableTest - it has mock expectation fails in PHPUnit8 due to callback return type should be bool and not string. Added a @todo and commented out the failing tests.

the patch

In #8, I misinterpreted the logic behind ExpectDeprecationTrait - its purpose is different than PHPUnit8 expectDeprecation, but we have a method name collision that we need to solve. Here, renaming to xxxExpectDeprecation.

Spinned off #3094151: ExpectDeprecationTrait is not compatible with PHPUnit 8.

Rerolled and added protection to prevent circular referencing of main/proxied methods that claims all memory and then fails - added an excpetion instead taht will call to action to rename the main methods in the test classes to their proxies.

Here's a megapatch converting ALL Drupal tests, including Kernel/Functional/JavaScript.

Here's a full patch rerolled, and a patch for review with just the key changes from HEAD - the changes are relatively small, the size of the full patch is due to thousands of renames.

Note: patches in #18 and #19 run as follows:

PHP 7 -> PHPUnit 6.5.14
PHP 7.1 -> PHPUnit 7.5.17
PHP 7.2 and 7.3 -> PHPUnit 8.4.3

@longwave re #24 void return types were introduced only in PHP 7.1, so adding the return typehint in Drupal 8, that still supports PHP 7.0, would lead to failure (see https://3v4l.org/gvBoi).

This issue is for D8. For D9, where min PHP is 7.2, that could be a good idea. But I think in that case we may want to drop PHPUnit 6 first to reduce the compatibility layer, and make PHPUnit 7 the default.

Can we announce there will be a BC break in tests in Drupal 9, or 9.1?

Looks like it will have to be, one way or another. Adding framework and release manager review tags to flag the issue.

Rerolled, after #3082340: Replace assertEquals() overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests with a MarkupInterfaceComparator implementation

Reroll after commit of #3102899: ViewExecutableTest uses a mocked argument callback wrongly (and fails in PHPUnit8).

Re-roll.

Trying a different approach, allowing BC, for #3094151: ExpectDeprecationTrait is not compatible with PHPUnit 8.

Disregard #34 and #35. Will re-roll from #33.

Rerolled from #33.

#39: no xxx is just a placeholder for "something descriptive" that I did not take care to think of, see IS.

What's happening upstream is only that some of the methods get a void return typehint in PHPUnit 8:

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -225,8 +225,8 @@
-  public static function setUpBeforeClass() {
+  public static function setUpBeforeClass(): void {

-  protected function setUp() {
+  protected function setUp(): void {
 
-  protected function assertPostConditions() {
+  protected function assertPostConditions(): void {

-    parent::assertPostConditions();
+    parent::assertPostConditions(): void;

-  protected function tearDown() {
+  protected function tearDown(): void {

and this 'forces' any extending class to comply to the typehint.

The patch here implements option 2 from the IS, which is, to some extent, black magic... TBH I would definitely prefer option 1 i.e. just add the return typehints, thus avoiding any PHPUnit dependent BaseTestCase class and the xxx prefixed methods which would be confusing for developers.

I do not see a problem for D9 core to move in that direction - since it is already only supporting PHP 7.2 and min PHP 7.3 support is on the way. The problem would be, though, in contrib, because all tests implementing the methods above would have to be adjusted for the return type hint which is only supported in PHP 7.1+ (see #25), so modules supporting both D8 and D9 would only be testable on PHP 7.1+.

IMHO we'll have to be pragmatic and accept that 'some' level of compatibility layer we will have to maintain anyway moving forward - for example, assertInternalType() will be removed from PHPUnit 9, and is deprecated in PHPUnit 8 - so we may want to have a FC shim in D9 while calls to assertInternalType() are converted to the recommended PHPUnit methods.

But here, yes, the impact of this return typehinting is huge and so far we could not find a softer BC solution.

FWIW, I tried to #3106802: Add void return typehint to test setUp methods in a contrib module, and results are as expected - pass on D8.8, D8.9 and D9.0 with PHP 7.1+, fail for PHP 7.0.

Updated IS with the fact that option #1 can also work for PHP 7.1.

Spinned off #3107732: Add return typehints to setUp/tearDown methods in concrete test classes to discuss option 1 for Drupal 9 in isolation.

So based on latest comments in #3107732: Add return typehints to setUp/tearDown methods in concrete test classes and #3106918: Drop support of PHPUnit 6 in Drupal 9 because it will never get used anyway I think we should stop targeting D8 and focus on D9 instead.

Postponed on #3107732: Add return typehints to setUp/tearDown methods in concrete test classes.

@alexpott kudos - at some point I fantasized about dynamically rewriting the base test class, but looking at your code I realise I would have never been able to. I guess this should be an option 3 in the IS, so flagging for IS update.

Couple of questions -
1) shall we need to emit a deprecation when the actual class being tested is missing the void return type from the relevant methods? Maybe a point for #3107732: Add return typehints to setUp/tearDown methods in concrete test classes, that would take a different path then.
2) PHPUnit 7 EOL is Feb 7, 2020, in 9 days, when PHPUnit 9 will be released. Would it make sense to jump to PHPUnit 8 (meant to support PHP 7.2, 7.3 and 7.4) directly and drop PHPUnit7 straight away?

Bumped #3094151: ExpectDeprecationTrait is not compatible with PHPUnit 8 to critical as it's become a blocker for this issue.

1. +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php
   @@ -0,0 +1,63 @@
   +    // If the class exists already there is nothing we can do. Hopefully this
   +    // is happening because this has been called already. The call from
   +    // \Drupal\Core\Test\TestDiscovery::registerTestNamespaces() necessitates
   +    // this protection and we only need that call for Simpletest UI so once
   +    // Simpletest is removed we can remove this.
   

   Add a @todo referencing #3075490: Move simpletest module to contrib?

2. +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php
   @@ -0,0 +1,63 @@
   +    // Mutate TestCase code to make it compatbile with Drupal 8 and 9 tests.
   

   compatbile/compatible

3. +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php
   @@ -0,0 +1,63 @@
   +    // Fixes Drupal\Tests\Core\Plugin\DefaultLazyPluginCollectionTest - probably
   +    // should fix the test instead.
   +    class_alias('PHPUnit\Framework\MockObject\Rule\InvocationOrder', 'PHPUnit\Framework\MockObject\Matcher\InvokedRecorder');
   

   Add a @todo and/or a followup?

4. +++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/RunnerVersion.php
   @@ -27,4 +27,43 @@ public static function getMajor() {
   +  /**
   +   * Mutates the TestCase class from PHPUnit to make it compatible with Drupal.
   +   *
   +   * @param $autoloader
   +   *   The autoloader.
   +   *
   +   * @throws \ReflectionException
   +   *
   +   * @internal
   +   *   This should only be called by test running code and this method will be
   

   shoudn't this be removed from RunnerVersion then?

I'd be +1 on RTBCing this.

#70 +1000 from a contrib maintainer pov. This patch allow modules currently specifying

core_version_requirement: ^8.8 || ^9

to test on PHP 7.0 through to PHP 7.4 with no major headache.

I was also experiencing the SQLite failure like in #66, in #30, and I think this boils down to the SQLite's Connection destructor

  /**
   * Destructor for the SQLite connection.
   *
   * We prune empty databases on destruct, but only if tables have been
   * dropped. This is especially needed when running the test suite, which
   * creates and destroy databases several times in a row.
   */
  public function __destruct() {
    if ($this->tableDropped && !empty($this->attachedDatabases)) {
      foreach ($this->attachedDatabases as $prefix) {
        // Check if the database is now empty, ignore the internal SQLite tables.
        try {
          $count = $this->query('SELECT COUNT(*) FROM ' . $prefix . '.sqlite_master WHERE type = :type AND name NOT LIKE :pattern', [':type' => 'table', ':pattern' => 'sqlite_%'])->fetchField();

          // We can prune the database file if it doesn't have any tables.
          if ($count == 0) {
            // Detaching the database fails at this point, but no other queries
            // are executed after the connection is destructed so we can simply
            // remove the database file.
            unlink($this->connectionOptions['database'] . '-' . $prefix);
          }
        }
        catch (\Exception $e) {
          // Ignore the exception and continue. There is nothing we can do here
          // to report the error or fail safe.
        }
      }
    }
  }

I was thinking to @ silence the unlink call that should not impact anything at that point.

Filed #3111044: Deprecate setUp/tearDown concrete test methods that do not specify void return type as a follow-up.

Time to RTBC this? I cannot, really...

+1 on #84, IMHO it wouldn't make sense to introduce more deprecation triggers in 9.0.x right now for removal before beta

Added a couple more issues to the CR.

Trying running the tests directky from PHPUnit on TravisCI leads to errors like

$ $DRUPAL_PATH/vendor/bin/phpunit $TEST_ARGS
PHP Warning:  file_put_contents(/home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/../../../../../../sites/simpletest/TestCase.php): failed to open stream: No such file or directory in /home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php on line 49
Warning: file_put_contents(/home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/../../../../../../sites/simpletest/TestCase.php): failed to open stream: No such file or directory in /home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php on line 49
PHP Warning:  include(/home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/../../../../../../sites/simpletest/TestCase.php): failed to open stream: No such file or directory in /home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php on line 51
Warning: include(/home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/../../../../../../sites/simpletest/TestCase.php): failed to open stream: No such file or directory in /home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php on line 51
PHP Warning:  include(): Failed opening '/home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/../../../../../../sites/simpletest/TestCase.php' for inclusion (include_path='/home/travis/drupal8/vendor/pear/archive_tar:/home/travis/drupal8/vendor/pear/console_getopt:/home/travis/drupal8/vendor/pear/pear-core-minimal/src:/home/travis/drupal8/vendor/pear/pear_exception:.:/home/travis/.phpenv/versions/7.4.2/share/pear') in /home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php on line 51
Warning: include(): Failed opening '/home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/../../../../../../sites/simpletest/TestCase.php' for inclusion (include_path='/home/travis/drupal8/vendor/pear/archive_tar:/home/travis/drupal8/vendor/pear/console_getopt:/home/travis/drupal8/vendor/pear/pear-core-minimal/src:/home/travis/drupal8/vendor/pear/pear_exception:.:/home/travis/.phpenv/versions/7.4.2/share/pear') in /home/travis/drupal8/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php on line 51
PHP Fatal error:  Declaration of Drupal\Tests\UnitTestCase::setUp() must be compatible with PHPUnit\Framework\TestCase::setUp(): void in /home/travis/drupal8/core/tests/Drupal/Tests/UnitTestCase.php on line 38
Fatal error: Declaration of Drupal\Tests\UnitTestCase::setUp() must be compatible with PHPUnit\Framework\TestCase::setUp(): void in /home/travis/drupal8/core/tests/Drupal/Tests/UnitTestCase.php on line 38

maybe the problem is the following piece of code

+++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php
@@ -0,0 +1,54 @@
+    $filename = __DIR__ . '/../../../../../../sites/simpletest/TestCase.php';
+    // Only write when necessary.
+    if (!file_exists($filename) || md5_file($filename) !== md5($alteredCode)) {
+      file_put_contents($filename, $alteredCode);
+    }
+    include $filename;

is not creating the simpletest directory if it's missing?

#92 fixes #91 for me.

Out of curiosity - PHPUnit manages its deprecations as (W)arnings in its CLI run. We do not see those in DrupalCI because they're not captured (#2905007: Allow run-tests.sh to report skipped/incomplete PHPUnit tests would address that).

As an example, running the test suite for --group=Database, we get

$ $DRUPAL_PATH/vendor/bin/phpunit $TEST_ARGS
PHPUnit 8.5.2 by Sebastian Bergmann and contributors.

Testing 
...............................................................  63 / 302 ( 20%)
...................W........W.................................. 126 / 302 ( 41%)
.....WWW....................................................... 189 / 302 ( 62%)
.........................................................S..... 252 / 302 ( 83%)
..............................S...................              302 / 302 (100%)

Time: 2.97 minutes, Memory: 835.00 MB

There were 5 warnings:

1) Drupal\Tests\Core\Database\ConnectionTest::testDestroy
assertAttributeEquals() is deprecated and will be removed in PHPUnit 9.
readAttribute() is deprecated and will be removed in PHPUnit 9.
getObjectAttribute() is deprecated and will be removed in PHPUnit 9.

/home/travis/drupal8/vendor/phpunit/phpunit/src/Framework/TestResult.php:691
/home/travis/drupal8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
/home/travis/drupal8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
/home/travis/drupal8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
/home/travis/drupal8/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
/home/travis/drupal8/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:621
/home/travis/drupal8/vendor/phpunit/phpunit/src/TextUI/Command.php:200
/home/travis/drupal8/vendor/phpunit/phpunit/src/TextUI/Command.php:159

2) Drupal\KernelTests\Core\Database\SelectTest::testVulnerableComment
Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9. Refactor your test to use assertStringContainsString() or assertStringContainsStringIgnoringCase() instead.

/home/travis/drupal8/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

3) Drupal\KernelTests\Core\Database\SelectSubqueryTest::testConditionSubquerySelect2
The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.

/home/travis/drupal8/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

4) Drupal\KernelTests\Core\Database\SelectSubqueryTest::testConditionSubquerySelect3
The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.

/home/travis/drupal8/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

5) Drupal\KernelTests\Core\Database\SelectSubqueryTest::testConditionSubquerySelect4
The optional $canonicalize parameter of assertEquals() is deprecated and will be removed in PHPUnit 9. Refactor your test to use assertEqualsCanonicalizing() instead.

/home/travis/drupal8/vendor/phpunit/phpunit/src/Framework/TestResult.php:691

WARNINGS!
Tests: 302, Assertions: 1770, Warnings: 5, Skipped: 2.