[meta] Support PHPUnit 9 in Drupal 9

Let's see what breaks... interdiff to the patch in #3063887: Support PHPUnit 8 in Drupal 9, drop support for PHPUnit 7

Repurposing this as a meta as suggested by @alexpott in #3108006: Replace assertInternalType() calls with dedicated methods

We should work out what we're doing for assertArraySubset - if you run core/tests/Drupal/KernelTests/Core/Action/SaveActionTest.php on D9 with -v you get warnings.

Updated dates in IS

assertArraySubset: In https://github.com/sebastianbergmann/phpunit/issues/3494, someone created a separate package, but that's PHPUnit 9 only and doesn't help to get rid of the deprecation messages :-/

So if I understand that correctly, per https://www.drupal.org/pift-ci-job/1587117, phpunit is currently writing out lots of warnings about its own deprecations and we ignore them unless these tests actually fail in a different way too.

I think the first step would be to improve run-tests.sh to fail on that and fix that? For BC, we could see if can do our own deprecation instead in overridden methods that we can control.

That's also what @mondrake meant in #3063887-93: Support PHPUnit 8 in Drupal 9, drop support for PHPUnit 7. It's not "travis stuff", it's core stuff, we're simply ignoring it right now.

As a quick hack, just letting all tests with warnings fail, to see how bad it is.

Both #3108006: Replace assertInternalType() calls with dedicated methods and #3113077: Replace assertContains() on strings with assertStringContainsString() or assertStringContainsStringIgnoringCase() are disruptive to Drupal 8.8/8.9/9.0 test compatibility as the replacement methods aren't available in PHPUnit 6, which is the minimum version for Drupal 8.8. We either need to add forward compatibility shims to Drupal 8.8/8.9 so we can use the newer methods, or I think we have to bump this to 9.1?

Drupal 8 doesn't support PHPUnit 8 I think, so isn't bothered by those deprecations in the first place?

I think it's fine to do these replacements only in 9.0.

But we want to be able to easily port patches between 8.9 and 9.0, so changing this in 9.0 only makes that more difficult.

That's true, but removing deprecations does make that harder too for example :) I think 9.0 vs 9.1 is up to to framework/release managers at this point, I originally thought it's better to wait for 9.1 with these changes and also the return type, but I think these warnings are going to be very confusing when running tests manually and it might be worth the extra overhead of keeping patches in sync. The 9.1 branch might open as early as march from what I know, so we'll need 9.1 compatible patches from then on anyway.

How about adding a forward compatibility layer for now - so we don't trigger the warnings and then we can fix later in D9. We could copy the PHPUnit 8 methods to our code and remove the warnings.

Here's a pragmatic approach - we can ignore the warnings. In the future we can change the warnings into our deprecations and remove usage from core but not when we're trying to make D8 and D9 cross compatibility easy.

+++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
@@ -209,14 +209,14 @@ public function runTests($test_id, array $unescaped_test_classnames, &$status =
-    if ($status == TestStatus::PASS) {
+    if ($status == TestStatus::PASS && strpos(implode('', $output), 'WARNINGS') === FALSE) {

So this is a bit brittle because we'll get into a problem with test classes that contain the work WARNINGS :). Fortunately we can get PHPUnit to convert the warnings to errors for us and therefore be a bit more strict. Yay.

The approach I've followed for fixing the component unit tests is as follows - I've fixed the tests as these don't change much - but I've used the trait for the composer tests because this array is under active development and using the trait is likely to make things a bit simpler.

A little neater.

1. +++ b/core/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php
   @@ -702,7 +702,7 @@ public function testAnnotationWithRequiredAttributesWithoutContructor()
   +            $this->assertStringContainsString('Attribute "value" of @Drupal\Tests\Component\Annotation\Doctrine\Fixtures\AnnotationWithRequiredAttributesWithoutContructor declared on property SomeClassName::invalidProperty. expects a(n) string. This value should not be null.', $exc->getMessage());
   
   @@ -1145,7 +1145,7 @@ public function testCastInt()
   +        $this->assertIsInt($annot->foo);
   

   How will we backport these to Drupal 8/PHPUnit 6, as these tests directly extend TestCase but these new methods don't exist in PHPUnit 6? Or will we just change these components in 9.x only?

2. +++ b/core/tests/Drupal/Tests/Traits/PHPUnit8Warnings.php
   @@ -0,0 +1,50 @@
   +   * Ignores specific PHPUNit 8 warnings.
   

   Nitpick: PHPUNit -> PHPUnit

@longwave none of this is intended for backport. I think it is ok if this component test diverges a bit - it's not like it changes alot. And the people we really want to help is contrib - so they can maintain tests on both D8 and D9 without that much complexity. Apply this patch to D9 achieves that.

Hopefully green.

I've split #33 out into a critical sub-issue as it really is about PHPUnit8 and not PHPUnit9. See #3116856: Workaround PHPUnit 8 warnings

Once this is complete any tests working with the assert statement can be configured not to run if assertions are turned off using the requires setting annotation.

I'm puzzled as to why this hasn't been added to the documentation for PHP Unit. Mr. Bergmann did the pull over 2 years ago. This feature was written by me specifically for when we switched over to handle this circumstance. Anyway, I checked the codebase of PHPUnit, both 8.5 and 9.1 have the code necessary to honor setting annotations.

/**
 * @requires setting zend.assertions 1
 */

That will cause the test to skip if assertions are turned off, useful for the tests in place to make sure runtime assertions are thrown when they should be. Conversely -1 can be used to skip the test if assertions are turned on.

Assertions aren't the only ini directive this can test for - any ini directive is fair game.

Why are we failing and aborting tests on DrupalCI that use a method that's *deprecated* in PHPUnit 8? These are still valid methods, PHPUnit 8 is still in use on DrupalCI. These should not cause a test fail. Instead they should be handled like methods in Drupal that are deprecated - tests should pass.

The PHPUnit-bridge deprecation handler and #3116856: Workaround PHPUnit 8 warnings are silencing the warnings, currently.

That's not true, at least for contrib.

It is not acceptable to fail tests running with PHPUnit 8 on DrupalCI just because of one method that will be removed in PHPUnit 9. Give me a notice if you want, or let me run deprecation checks myself like we do for all other deprecations, but failing the tests and forcing me to immediately change my code is wrong - If PHPUnit 8 is still supported, then you must support it.

See https://www.drupal.org/node/2632588/qa for the failure.