Problem/Motivation

Two recent backports both broke PHP 5 testing (but not runtime):

  1. #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning
  2. #3007102: Migrating to Date-only field does not drop time value

Per @Mixologic:

The backport introduced some php5.5 and php5.6 regressions:

expectException doesnt exist in phpunit 4, so the backport tests fail.

https://www.drupal.org/pift-ci-job/1397533

I see a lot of this pattern in core currently:

    if (method_exists($this, 'expectException')) {
      $this->expectException(\BadMethodCallException::class);
      $this->expectExceptionMessage('Method Drupal\Component\Datetime\DateTimePlus::diff expects parameter 1 to be a \DateTime or \Drupal\Component\Datetime\DateTimePlus object');
    }
    else {
      $this->setExpectedException(\BadMethodCallException::class, 'Method Drupal\Component\Datetime\DateTimePlus::diff expects parameter 1 to be a \DateTime or \Drupal\Component\Datetime\DateTimePlus object');
    }

Which seems to be the correct workaround to address those.

Proposed resolution

The two issues require the same fix and neither will pass tests without the other being fixed too. So, do that here.

We have two possible solutions so far on the two related issues:

  1. Add a try/catch like so:
    +++ b/core/modules/datetime/tests/src/Unit/Plugin/migrate/field/DateFieldTest.php
    @@ -83,8 +83,13 @@ public function testDefineValueProcessPipelineException() {
    -    $this->expectException(MigrateException::class);
    -    $plugin->defineValueProcessPipeline($migration, 'field_date', ['type' => 'totoro']);
    +    try {
    +      $plugin->defineValueProcessPipeline($migration, 'field_date', ['type' => 'totoro']);
    +      $this->fail("Expected MigrateException was not thrown");
    +    }
    +    catch (\Exception $e) {
    +      $this->assertInstanceOf(MigrateException::class, $e);
    +    }
       }
    
  2. Switch back to setExpectedException() like so:
    +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -127,8 +126,7 @@ public function testMissingCoreCoreVersionRequirement() {
    -    $this->expectException('\Drupal\Core\Extension\InfoParserException');
    -    $this->expectExceptionMessage("$exception_message-duplicate.info.txt");
    +    $this->setExpectedException(InfoParserException::class, "$exception_message-duplicate.info.txt");
    

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm credited Mixologic.

xjm credited mikelutz.

xjm credited tedbow.

xjm’s picture

mikelutz’s picture

->setExpectedException is correct I believe. Both these classes extend UnitTestCase, which implement PhpUnitCompatibilityTrait, which override ->setExpectedException to call ->expectedException on phpunit 6, so that's the right way to go.

mikelutz’s picture

Status: Active » Needs review
mikelutz’s picture

xjm’s picture

Thanks @mikelutz -- I'm just going to commit this from NR since HEAD has now been broken for two weeks and I want to stop getting emails. :P

5.6 has a very long history of segfaulting on random stupid things like how an array is sorted; that segfault has been ongoing for more than a year and is one of the reasons I wanted to actually skip the test on the 5.6 nightly for 8.7. :P And also part of why I am so looking forward to December when we can say goodbye to those test runs permanently. :)

  • xjm committed ddcd252 on 8.7.x
    Issue #3079805 by mikelutz, xjm, Mixologic, tedbow: expectedException()...
xjm’s picture

Status: Needs review » Fixed

Ta-da!

Status: Fixed » Closed (fixed)

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