RFC: Deprecate SapphireTest (was: Split unit testing support into silverstripe/test-support)

[sminnee]

EDIT: I've updated this with a very different recommendation:

Right now we're running all our tests on PHPUnit 5. If we want PHPUnit support, we either need to allow the use of PHPUnit 9, or we need to fork PHPUnit 5 and fix it to work with PHP 8, which is likely to be relatively straightforward, but requires the establishment and maintenance of a fork as I have with http://github.com/sminnee/phpunit-mock-objects.

It's worth noting that users of silverstripe/framework needn't care about how the framework's tests are run, they only need to know that their own tests would keep running.

Although I had originally proposed splitting things into a lot of packages, I think there's a better path: deprecate SapphireTest because requiring a common ancestor like this is an anti pattern.

Without any further help, I would imagine that a test might look like this:

use PHPUnit\Framework\TestCase;
use SilverStripe\Dev\SapphireTestState;

class MyTest extends TestCase {
    public static function setUp()
    {
        SapphireTestState::inst()->setUp();
    }

    public static function tearDown()
    {
        SapphireTestState::inst()->tearDown();
    }

    public static function setUpBeforeClass()
    {
        SapphireTestState::inst()->setUpOnce();
    }

    public static function tearDownAfterClass()
    {
        SapphireTestState::inst()->tearDowOnce();
    }

    // ... boilerplate done, tests can continue
}

However, we could potentially provide that boilerplate in a trait rather than a base class. By making it a trait we could provide PHPUnit 5 and 8+ (or 7+?) versions, if we wanted:

use PHPUnit\Framework\TestCase;
use SilverStripe\Dev\PHPUnit5TestState;

class MyTest extends PHPUnit_Framework_TestCase {
    use PHPUnit5Test;
}

But to be honest, providing a trait might be overkill, as it makes things like writing the tests own setUp() method harder, especially if the SapphireTestState logic could be smart enough to avoid the need for the once methods.

General approach recommended:

• Refactor the meat of SapphireTest into SapphireTestState and plugins in a PR.
• Pull that PR into a project, eg, with composer-patches. Try updating a project's test suite to avoid the use of SapphireTest. Try upgrading the project to PHPUnit 9. Report back on dev experience, refine as needed.
• Update test documentation in the PR recommending the new approach
• Merge the PR

At this point, we'd allow other versions of PHPUnit on projects, but the recipe's modules would still be constrained by kitchen-sink tests.

• Consider refactoring kitchen-sink tests so that different modules can have different testing frameworks.
• Update framework to use the new style and attempt to upgrade to PHPUnit 7 || 8 || 9 (to retain PHP 7.1 support)
• Update other modules

[sminnee]

Having written all this down, I'm now thinking "maybe for starters I'll just publish a fork of phpunit 5.7" :P

CC @Cheddam

[sminnee]

OK that's published with a single version - 5.7.28 - http://github.com/sminnee/phpunit and https://packagist.org/packages/sminnee/phpunit

#9665 updates the require-dev requirements to pull this in, among other things.

[sminnee]

OK having thought about this more I think that supporting PHPunit 8/9 would be better done with SilverStripe/dev-support:^2, but that such a version would be designed to work with framework 4

[Acen]

Was looking at making an RFC around this too.

Were there any specific reasons around why the version was locked off to 5.7 initially? Considering support dropped off from phpunit themselves at the beginning on 2018.

[sminnee]

You can't do a major bump without breaking people's tests. So current projects need to be able to keep using 5.7 if they wish.

But if we altered SapphireTest to support other versions, then we would drop support for phpunit 5.7

Decoupling SapphireTest et al from the rest of framework in dev-support package means that we could use dev-support 2 for testing the framework itself but let existing projects keep using dev-support 1.

[robbieaverill]

We also need phpunit 5.x because it supports PHP 5.6, which we still support

[sminnee]

Nah we're supporting 7.1+ these days, so we could get away with using phpunit 7 for the core suite and that of supported modules, if we solved the underlying issue.

A release of dev-support that supports PHPUnit 7 | 8 | 9 would be hard, because the setUp and tearDown methods require different signatures in 7 vs 8. But I would pick that up as a design challenge / decision if and when dev-support was split out as a separate package.

[robbieaverill]

Oh that helps a lot!

[sminnee]

OK I've put a bit more detail on what such a module-split might look like.

[sminnee]

From @dhensby on the PHP 8 support PR

It's a shame we can't just upgrade to PHPUnit 8, but there has been some work to the ss-upgrader tool to add an easier upgrade path for that in SS-5.

This RFC is what I've got in mind for that.

[sminnee]

One thing I want to highlight here:

Having start on the module-splitting work I think SapphireTest and the other direct PHPUnit bindings are the only that should go into a separate module. The rest should be left in framework, or in the decoupled modules I've been PoCing recently.

So I think the module should be called silverstripe/phpunit-support.

Also: most of the "meat" of SapphireTest should be shifted to more teststate implementations.

This would make it easier to get the benefits of the test system with other test frameworks too, as well as newer phpunit versions.

I'm advocating leaving teststate in core as ORM, Control, Security, etc will have their own state contributions. Pulling these pieces out into a dev module (or multiple dev modules such as dev-orm) will increase coupling.

[sminnee]

Giving this more thought, the best first step on this task is probably to:

• Refactor the guts of SapphireTest into the TestState system
• Trial the developer experience of project running PHPUnit 9 and avoiding the use of SapphireTest entirely

If it works well it might be easier to simply deprecate the use of SapphireTest altogether, rather than have a module that provides SapphireTest. This would also make it easier to use other test frameworks on projects, such as Codeception.

For modules that are part of recipe-core, recipe-cms, and cwp-recipe-kitchen-sink, we still assumed that the same test tooling is used for all modules. We may want to try decoupling that, but it will be hard.

[Cheddam]

A quick update on this after discussion with @sminnee:

We've run into additional issues supporting PHP 8 on the sminnee/phpunit forks, and are unlikely to perform any further maintenance on them, as getting the CI green again will require forking further pieces of the phpunit ecosystem, and this is an unsustainable and ultimately unhelpful rabbit hole to continue traversing.

This means that PHP 8 support in 4.x will continue to require use of the --ignore-platform-reqs flag for the time being, and any further edgecases or bugs encountered in the sminnee/phpunit forks are unlikely to receive fixes.

It's becoming increasingly important to ratify this RFC and proceed with development of PHPUnit 7 / 8 / 9 support in the 4.x line, in order to enable running PHP 8 with no compromises.

[emteknetnz]

I'll have a ramble about some problems I see. Apologies in advance, I don't have any solutions here.

PHP8 support is desirable, but migrating tests seems hard.
I do recognise the need go on to new versions of phpunit, in particular ^9.3 which includes php8 support. This is probably the most signficant hurdle to overcome before we can say that Silverstripe has full PHP8 support which means not using the --ignore-platform-reqs flag.

However, the biggest hurdle seems to be migration all the existing silverstripe modules from SapphireTest to a new test system. You know, all the modules with hundreds of *Test.php files that use SilverStripe\Dev\SapphireTest; ... class SomeClassTest extends SapphireTest, well they all need to be updated now.

You cannot composer install two different versions of phpunit i.e. install 5.7 AND 9.3. This would be pretty useful from a migration point of view so that you can run all the silverstripe unit tests in the vendor folder whether they're written in the new ^9.3 format, or in the old ^5.7 SapphireTest format. But you can't do this, so unless you could magically update ALL modules at once, a bunch of your unit tests won't be runnable depending on the version on phpunit you have installed.

Also, will things even be installable/dev-buildable if you have ^9.3 installed because it seems like SapphireTest won't actually exist? Would the php interpreter complain about all the unit test classes that extends SapphireTest ?

SapphireTest.php

if (!class_exists(PHPUnit_Framework_TestCase::class)) {
    return;
}
class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly

Forking and technical debt
The easier short-term solution that require no migration is just fork all the relevant phpunit ^5.7 dependencies so they work with PHP8. This seems like signficantly less work and requires no migration of tests for silverstripe modules.

However there's no denying that we're just taking out further technical debt here. At some point PHP 7 will be just like PHP 5 and we'll be forced to upgrade.

Deprecation notices
I don't see deprecating SapphireTest as achieving a whole lot. The module resides in framework so it would only be removed with the release of Silverstripe 5, which as we know is some ways off. It seems like all this will do in practical terms is spit a bunch of annoying deprecation notices when running unit tests.

Apologies again for only providing problems and not solutions.

[sminnee]

One thing about this ticket is that it's not just, or even primarily, about the framework's test themselves – one of the big benefits of this proposal is that it lets projects use newer / alternative testing frameworks, which I think would be useful.

The issues you raise about the core basically come down to any kind of change management, and are separate: "can I use PHPUnit 7||8||9" is different from "should I migrate these 90 module to use it". This ticket is really focused on the first one.

I agree that "deprecation" might be a "little-d deprecation" in that we have better recommended ways for people to write tests (and e.g. we update our docs to avoid discussion of SapphireTest). Formal deprecation should only come after the core and a good set of modules stopped using it.

I'll shift the discussion of PHP8 support to your separate ticket.