[WIP] API Upgrade SapphireTest to work with phpunit 9 #10028

emteknetnz · 2021-07-21T00:45:48Z

Just a POC at this point

Notes:

Trying to make migration for existing projects as simple as possible
One of the major changes for tests is that setUp() and tearDown() methods now both require a : void return type
Doing if (!class_exists(PHPUnit_Framework_TestCase::class)) { on _legacy/SapphireTest and if (class_exists(TestCase::class)) { so that _legacy/SapphireTest is what's used for function signature validation by IDE's and also php itself. This is because return types support covariance (become more specific) but not contravariance (become more abstract). A missing return type (_legacy style) can be made more specific with :void, but not vice versa.

This is handy to get composer to install

    "replace": {
        "sminnee/phpunit": "*",
        "sminnee/phpunit-mock-objects": "*"
    },

Migration notes:

Running the following regex find and replaces

Regex

(protected|public) function setUp\(\)\n
protected function setUp(): void\n

(protected|public) function tearDown\(\)\n
protected function tearDown(): void\n

public static function setUpBeforeClass\(\)\n
public static function setUpBeforeClass(): void\n

public static function tearDownAfterClass\(\)\n
public static function tearDownAfterClass(): void\n

"(sminnee|phpunit)/phpunit": "\^5.+?(,*)\n
"phpunit/phpunit": "^9"$1\n

"sminnee/phpunit-mock-objects":.+?\n( +)
$1

"phpunit/phpunit": "\^9",([\n ]+ )\}
"phpunit/phpunit": "^9"$1}

String replace

$this->assertInternalType('numeric', 
$this->assertIsNumeric(

$this->assertInternalType('int', 
$this->assertInternalType('integer', 
$this->assertIsInt(

$this->assertInternalType('double', 
$this->assertInternalType('float', 
$this->assertInternalType('real', 
$this->assertIsFloat(

$this->assertInternalType('string', 
$this->assertIsString(

$this->assertInternalType('boolean', 
$this->assertInternalType('bool', 
$this->assertIsBool(

$this->assertInternalType('null', 
$this->assertTrue(null === 

$this->assertInternalType('array', 
$this->assertIsArray(

$this->assertInternalType('object', 
$this->assertIsObject(

$this->assertInternalType('resource', 
$this->assertIsResource(

$this->assertInternalType('resource (closed)', 
$this->assertIsClosedResource(

$this->assertInternalType('scalar', 
$this->assertIsScalar(

$this->assertInternalType('callable', 
$this->assertIsCallable(

$this->assertInternalType('iterable', 
$this->assertIsIterable(

$this->assertFileNotExists(
$this->assertFileDoesNotExist(

$this->assertRegExp(
$this->assertMatchesRegularExpression(


String replace then audit

$this->assertContains
$this->assertStringContainsString

$this->assertNotContains
$this->assertStringNotContainsString

Require the most work, as non-iterable haystacks (strings) now need to be called with assertStringContainsString/assertNotStringContainsString. Iterable haystacks (arrays, etc) stay as are. You'll need to find all instance and manually assess whether they should be changed or not.

Since there's probably going to be way more string haystacks than iterable haystacks, do the string replace then search for the replacement and revert the ones with an iterable haystack

Any <testsuite> in a phpunit.xml.dist file now needs to wrapped in <testsuites>

@expectedException docblock annotation no longer work. They need to be replaced with the following:

PHPUnit exceptions - forwards compatible version has been added to _legacy/SapphireTest.php if still using phpunit5
PHPUnit_Framework_Error => $this->expectError()
PHPUnit_Framework_Error_Warning => $this->expectWarning()
PHPUnit_Framework_Error_Notice => $this->expectNotice()
PHPUnit_Framework_Error_Deprecated => $this->expectDeprecated()

Asserting warnings for phpunit exceptions are handled with

$this->expectWarningMessage() and $this->expectWarningMessageMatches()

All other exceptions are handled with

$this->expectException(LogicException::class);

@expectedExceptionMessage => $this->expectExceptionMessage($message)
@expectedExceptionMessageRegExp => $this->expectExceptionMessageMatches($regex)
@expectedExceptionCode => $this->expectExceptionCode($code)

The following searches will find the things
@expectedException PHPUnit_Framework_

Followed by
@expectedException

assertArraySubset() has been removed. It can be polyfiled for backwards compatibility on individual SapphireTest classes with a trait in a module:

$this->assertArraySubset

composer require --dev dms/phpunit-arraysubset-asserts

"dms/phpunit-arraysubset-asserts": "^0.3.0",

use DMS\PHPUnitExtensions\ArraySubset\ArraySubsetAsserts;

class MyTest extends SapphireTest
{
    use ArraySubsetAsserts;

Have added as a dev dependency to framework
Decided not to put this trait on SapphireTest for Semver. This way gives us the ability to later migrate off using this function and remove the requirement. In patch notes we could just say re-require it on your project if needed.

MessageRegExp( => MessageMatches(
assertRegExp( => assertMatchesRegularExpression(
assertNotRegExp( => assertDoesNotMatchRegularExpression(

Update composer

require-dev: {
  "phpunit/phpunit": "^9" / "silverstripe/recipe-testing": "dev-two"

Still need to update docs/en/02_Developer_Guides/06_Testing/00_Unit_Testing.md

maxime-rainville

We'll need to validate how our API doc process the dual classes.

We probably need to have some migration guide somewhere. But that can probably be another task.

composer.json

docs/en/02_Developer_Guides/06_Testing/00_Unit_Testing.md

src/Dev/Constraint/SSListContains.php

src/Dev/SapphireTest.php

tests/behat/src/CmsFormsContext.php

tests/php/Control/IPUtilsTest.php

tests/php/Core/Manifest/ManifestFileFinderTest.php

tests/php/Core/Manifest/NamespacedClassManifestTest.php

tests/php/Core/PathTest.php

tests/php/Forms/DateFieldTest.php

tests/php/View/ArrayDataTest.php

tests/php/View/HTMLTest.php

tests/php/View/Parsers/ShortcodeParserTest.php

tests/php/View/SSViewerTest.php

src/Dev/Constraint/SSListContains.php

src/Dev/Constraint/SSListContainsOnlyMatchingItems.php

maxime-rainville · 2021-10-26T02:28:55Z

src/Dev/SapphireTest.php

    }

-    public static function assertNotContains(
+    public static function assertNotStringContainsString(


Why change signatures of legacy methods?

You should not be changing the legacy class

maxime-rainville · 2021-10-26T02:32:05Z

src/Dev/SapphireTest.php

@@ -639,7 +1963,7 @@ public static function assertNotContains(
        if ($haystack instanceof DBField) {
            $haystack = (string)$haystack;
        }
-        parent::assertNotContains($needle, $haystack, $message, $ignoreCase, $checkForObjectIdentity, $checkForNonObjectIdentity);
+        parent::assertNotStringContainsString($needle, $haystack, $message, $ignoreCase, $checkForObjectIdentity, $checkForNonObjectIdentity);


Why change signatures of legacy methods?

You should not be changing the legacy class

maxime-rainville · 2021-10-26T02:32:52Z

src/Dev/SapphireTest.php

@@ -993,7 +2317,7 @@ public static function assertSQLNotContains(
        $needleSQL = static::normaliseSQL($needleSQL);
        $haystackSQL = static::normaliseSQL($haystackSQL);

-        static::assertNotContains($needleSQL, $haystackSQL, $message, $ignoreCase, $checkForObjectIdentity);
+        static::assertNotStringContainsString($needleSQL, $haystackSQL, $message, $ignoreCase, $checkForObjectIdentity);


Why change signatures of legacy methods?

You should not be changing the legacy class

tests/php/Core/Manifest/NamespacedClassManifestTest.php

tests/php/ORM/ArrayListTest.php

maxime-rainville

There's still some legacy classes that are seeing needless signature changes.

The current set up will allow you to install any version of PHPUnit. Still think need to address that in some way.

There's also a few comments that got miss. But nothing major.

maxime-rainville

Some of the legacy class implementation still differ from the original ... that's the only thing I want fix before we merge this.

We should have a follow up task to:

Disallow PHPUnit 6, 7 and 8
Add some sort of CI to make sure everything still works with when you run test with Sam's PHPUnit 5.7 fork.

src/Dev/Constraint/ViewableDataContains.php

maxime-rainville · 2021-10-26T20:49:49Z

src/Dev/SapphireTest.php

@@ -614,7 +1938,7 @@ protected function tearDown()
        static::$state->tearDown($this);
    }

-    public static function assertContains(
+    public static function assertStringContainsString(


You should not be changing the legacy class

maxime-rainville · 2021-10-26T20:51:47Z

src/Dev/SapphireTest.php

@@ -625,10 +1949,10 @@ public static function assertContains(
        if ($haystack instanceof DBField) {
            $haystack = (string)$haystack;
        }
-        parent::assertContains($needle, $haystack, $message, $ignoreCase, $checkForObjectIdentity, $checkForNonObjectIdentity);
+        parent::assertStringContainsString($needle, $haystack, $message, $ignoreCase, $checkForObjectIdentity, $checkForNonObjectIdentity);


You should not be changing the legacy class

maxime-rainville · 2021-10-26T20:52:13Z

src/Dev/SapphireTest.php

    }

-    public static function assertNotContains(
+    public static function assertNotStringContainsString(


You should not be changing the legacy class

maxime-rainville · 2021-10-26T20:52:23Z

src/Dev/SapphireTest.php

@@ -639,7 +1963,7 @@ public static function assertNotContains(
        if ($haystack instanceof DBField) {
            $haystack = (string)$haystack;
        }
-        parent::assertNotContains($needle, $haystack, $message, $ignoreCase, $checkForObjectIdentity, $checkForNonObjectIdentity);
+        parent::assertNotStringContainsString($needle, $haystack, $message, $ignoreCase, $checkForObjectIdentity, $checkForNonObjectIdentity);


You should not be changing the legacy class

maxime-rainville · 2021-10-26T20:52:40Z

src/Dev/SapphireTest.php

@@ -971,7 +2295,7 @@ public static function assertSQLContains(
        $needleSQL = static::normaliseSQL($needleSQL);
        $haystackSQL = static::normaliseSQL($haystackSQL);

-        static::assertContains($needleSQL, $haystackSQL, $message, $ignoreCase, $checkForObjectIdentity);
+        static::assertStringContainsString($needleSQL, $haystackSQL, $message, $ignoreCase, $checkForObjectIdentity);


You should not be changing the legacy class

maxime-rainville · 2021-10-26T20:52:52Z

src/Dev/SapphireTest.php

@@ -993,7 +2317,7 @@ public static function assertSQLNotContains(
        $needleSQL = static::normaliseSQL($needleSQL);
        $haystackSQL = static::normaliseSQL($haystackSQL);

-        static::assertNotContains($needleSQL, $haystackSQL, $message, $ignoreCase, $checkForObjectIdentity);
+        static::assertNotStringContainsString($needleSQL, $haystackSQL, $message, $ignoreCase, $checkForObjectIdentity);


You should not be changing the legacy class

maxime-rainville · 2021-10-26T23:03:42Z

tests/php/ORM/ArrayListTest.php

    public function testZeroLimit()
    {
+        $this->markTestIncomplete();


I would rather merge the test broken or remove the test altogether.

Leaving a comment like that is just a way of saying we will never fix the issue.

emteknetnz · 2021-10-27T00:05:20Z

@maxime-rainville I've added a composer conflict for phpunit 6 7 and 8

Have implemented all other peer review feedback

maxime-rainville

Good enough ... let's merge it and see what breaks!

emteknetnz force-pushed the pulls/4/sapphire-test-nine branch 3 times, most recently from 32377d9 to bcb2104 Compare July 21, 2021 05:54

emteknetnz mentioned this pull request Jul 21, 2021

PHPUnit 9+ (for PHP 8 support) #10019

Closed

93 tasks

emteknetnz force-pushed the pulls/4/sapphire-test-nine branch 26 times, most recently from eb91ac6 to 5d0e761 Compare July 23, 2021 23:34

emteknetnz force-pushed the pulls/4/sapphire-test-nine branch 2 times, most recently from 69b824e to e53c6de Compare October 21, 2021 00:13

maxime-rainville requested changes Oct 21, 2021

View reviewed changes

emteknetnz force-pushed the pulls/4/sapphire-test-nine branch 12 times, most recently from de7a2b2 to b7f71b3 Compare October 24, 2021 01:56

emteknetnz marked this pull request as ready for review October 26, 2021 02:01

maxime-rainville reviewed Oct 26, 2021

View reviewed changes

maxime-rainville requested changes Oct 26, 2021

View reviewed changes

emteknetnz force-pushed the pulls/4/sapphire-test-nine branch 2 times, most recently from af3c355 to bd3d6a8 Compare October 26, 2021 03:46

maxime-rainville requested changes Oct 26, 2021

View reviewed changes

API Upgrade SapphireTest to work with phpunit 9

fa5f7cc

emteknetnz force-pushed the pulls/4/sapphire-test-nine branch from bd3d6a8 to fa5f7cc Compare October 27, 2021 00:05

maxime-rainville approved these changes Oct 27, 2021

View reviewed changes

maxime-rainville merged commit cd07654 into silverstripe:4 Oct 27, 2021

maxime-rainville deleted the pulls/4/sapphire-test-nine branch October 27, 2021 02:40

This was referenced Aug 9, 2022

Remove support for PHPUnit 5.7 #10398

Closed

API Remove PHPUnit 5.7 compatability hacks #10444

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] API Upgrade SapphireTest to work with phpunit 9 #10028

[WIP] API Upgrade SapphireTest to work with phpunit 9 #10028

emteknetnz commented Jul 21, 2021 •

edited

maxime-rainville left a comment

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

maxime-rainville left a comment

maxime-rainville left a comment

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

maxime-rainville Oct 26, 2021

emteknetnz Oct 26, 2021

emteknetnz Oct 27, 2021

emteknetnz commented Oct 27, 2021

maxime-rainville left a comment •

edited

[WIP] API Upgrade SapphireTest to work with phpunit 9 #10028

[WIP] API Upgrade SapphireTest to work with phpunit 9 #10028

Conversation

emteknetnz commented Jul 21, 2021 • edited

Notes:

Migration notes:

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville left a comment

Choose a reason for hiding this comment

maxime-rainville left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz commented Oct 27, 2021

maxime-rainville left a comment • edited

Choose a reason for hiding this comment

emteknetnz commented Jul 21, 2021 •

edited

maxime-rainville left a comment •

edited