Problem/Motivation

Following #3252010: Lock PHPUnit to 9.5 Drupal 10 supports PHPUnit 9 only. There are some version-specific code sections that handle various cases in PHPUnit 8, these can now be removed.

Steps to reproduce

Proposed resolution

Remaining tasks

Decide what to do about the drupal-phpunit-upgrade script, as this is no use for now but may be needed for PHPUnit 10.

In #3252010-19: Lock PHPUnit to 9.5. it was decided to keep the drupal-phpunit-upgrade script.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#23 interdiff_21_23.txt2.04 KBbeatrizrodrigues
#23 3252257-23.patch13.47 KBbeatrizrodrigues
#21 interdiff_11-21.txt2.78 KBSpokje
#21 3252257-21.patch13.68 KBSpokje
#17 3252257-17.patch13.95 KBSpokje
#17 interdiff_11-17.txt3.04 KBSpokje
#14 interdiff_11-13.txt2.75 KBSpokje
#14 3252257-13.patch13.65 KBSpokje
#11 interdiff+9-11.txt8.17 KBSpokje
#11 3252257-11.patch11.49 KBSpokje
#10 3252257-9.patch2.71 KBpaulocs
#2 3252257-2.patch7.89 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Active » Needs review
FileSize
3252257-2.patch7.89 KB

There might be more that I missed, but this can all be safely removed at least.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Issue tags: +PHPUnit 10

We need to do #3182103: Ensure correct signature for setUp(), tearDown(), setUpBeforeClass(), and assertPostConditions() methods beforehand, I suppose.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

And in any case, I think we cannot do that if we want to keep a Symfony FC polyfill for upcoming PHPUnit versions.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented

Ah yeah we need to keep parts of the ClassWriter for future compatibility. Should we move it to a neutral namespace though, so we can delete the PhpUnit8 namespace?

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

#5 yes that makes sense and is in line with a comment that I saw somewhere by @alexpott that I cannot find right now. I'd suggest to just step one back and put the class in the Drupal\TestTools\PhpUnitCompatibility.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Needs review » Postponed
Related issues: +#3182103: Ensure correct signature for setUp(), tearDown(), setUpBeforeClass(), and assertPostConditions() methods

Postponed on #3182103: Ensure correct signature for setUp(), tearDown(), setUpBeforeClass(), and assertPostConditions() methods

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Postponed » Needs work

Unpostponing since #3182103: Ensure correct signature for setUp(), tearDown(), setUpBeforeClass(), and assertPostConditions() methods was committed.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Needs work » Needs review

New patch that addresses #6.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
FileSize
3252257-9.patch2.71 KB
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
FileSize
3252257-11.patch11.49 KB
interdiff+9-11.txt8.17 KB

some more candidates

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Needs review » Needs work

Thanks, this is much better than my attempt in #2.

One more change that I think is safe:

// In order to manage different implementations across PHPUnit versions, we
// dynamically load the base ResultPrinter class.
if (!class_exists(ResultPrinterBase::class, FALSE)) {
  class_alias('PHPUnit\TextUI\DefaultResultPrinter', ResultPrinterBase::class);
}

...

class HtmlOutputPrinter extends ResultPrinterBase {

We can drop the class alias and extend the DefaultResultPrinter directly now.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

#12 hmpf, I agree it's better to clean up now, even if when PHPUnit10 will be out some swapping will still be necessary. But I do not think we can anticipate that yet.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
FileSize
3252257-13.patch13.65 KB
interdiff_11-13.txt2.75 KB

Thanks @longwave, was already pondering if this class alias was needed any more, removed in attached patch.

Also removed PHPUnit 8 deprecations in \Drupal\Tests\Listeners\DeprecationListenerTrait.

Let's see how TestBot likes it...

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented

Decide what to do about the drupal-phpunit-upgrade script, as this is no use for now but may be needed for PHPUnit 10.

Personally I'm a fan of removing it. Unused code = dead code = gone IMHO.
If re-needed, it can be archaeologized back to life from the repo.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Needs work » Needs review

@mondrake yeah I couldn't easily decide, but from what I have seen the PHPUnit 10 event system will mean even more of a rewrite is required here, so we don't know yet whether this layer will be useful or if we will need something more complicated, and as @Spokje points out we can always resurrect it from git history if we do need it.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Status: Needs review » Needs work
FileSize
interdiff_11-17.txt3.04 KB
3252257-17.patch13.95 KB
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

re #15 please leave the update script in, see also #3252010-19: Lock PHPUnit to 9.5.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Issue summary: View changes
Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
Issue summary: View changes

Thanks @mondrake, updated IS to reflect the fact that the drupal-phpunit-upgrade script will remain.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje commented
FileSize
3252257-21.patch13.68 KB
interdiff_11-21.txt2.78 KB
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented 
  1. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -125,12 +125,10 @@ public static function getSkippedDeprecations() {
+      // PHPUnit.

    This is all PHPUnit 9 then now. Please leave it in the comment so that in the future when PHPUnit 9 will go away, we know we can remove the entire section.

  2. +++ /dev/null
@@ -1,122 +0,0 @@
-  /**
-   * Tests that selected PHPUnit warning is converted to deprecation.
-   */
-  public function testAddWarning() {
-    $this->expectDeprecation('Test warning for \Drupal\Tests\PhpUnitWarningsTest::testAddWarning()');
-    $this->addWarning('Test warning for \Drupal\Tests\PhpUnitWarningsTest::testAddWarning()');
-  }
-

    This should remain because it's testing the conversion of PHPUnit warning to deprecations in general regardless of the version.

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
CreditAttribution: beatrizrodrigues at CI&T commented
FileSize
3252257-23.patch13.47 KB
interdiff_21_23.txt2.04 KB

I'm sending a new patch that addresses #22.

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
CreditAttribution: beatrizrodrigues at CI&T commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Re. #12, I think it's OK as work in #3217904: [meta] Support PHPUnit 10 in Drupal 11 shows how the implementation of event subscribers will not cross at all with existing listeners, they will be rewritten in totally separate namespaces.

I went through it again and I think it's good to go.

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -125,12 +125,10 @@ public static function getSkippedDeprecations() {
+      // PHPUnit 9

dot missing at the end of the comment, this could also be fixed on commit. Strange phpcs doesn't complain.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Fixed the full stop on commit.

Committed eb1adb0 and pushed to 10.0.x. Thanks!

  • alexpott committed eb1adb0 on 10.0.x 
    Issue #3252257 by Spokje, beatrizrodrigues, longwave, paulocs, mondrake...

Status: Fixed » Closed (fixed)

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

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

This broke the compatibility class rewriter by not fixing the hard coded directory path to the simpletest directory. As noted int he follow up, not sure how this wasn't caught but its a simple enough fix.

#3259158: PhpUnit compatibility broken

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

We forgot a bit, opened #3261262: Remove PHPUnit 8 warnings conversion to deprecations.