Use a PHP8 compatible version of phpunit

[emteknetnz]

AC's

• Be able to able to install phpunit on a local environment running PHP8 without the using of the --ignore-platform-reqs flag
• (Strong preference) Do not require modification to any existing module unit tests

Options:

A) Update the sminnee fork of phpunit 5.7 to be php8 compatible, this may require also forking some downstream dependencies

B) Upgrade phpunit to ^9.3 which includes PHP8 support

There's an extended discussion here

[sminnee]

Option A will only be feasible with --ignore-platform-reqs IMO, unless you want to fork the entire dependency chain of phpunit. I really don't think that's a good use of time :-/

Option B will require changing tests but can hopefully be doable module-at-a-time. The hardest thing will be kitchen sink tests, though, which assume commonly-installed dev depencies.

I've also seen some projects that alter the tests with a simple regex rewrite in CI (adding void return type hints to setUp and tearDown) which might be doable as a bridging solution. It might also be possible to package that up as a script to run on each module.

[sminnee]

Erm, Perhaps closing this ticket and editing the body of my one to incorporate your ACs would be better than fracturing the discussion? I missed your reply on the original ticket ;-)

EDIT: Scratch that, this legitimately belongs as a separate issue.

[sminnee]

From @emteknetnz on #9668

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 ?

I think we need to be clear about where it's important to drop --ignore-platform-reqs.

• If your goal is to drop it from projects, then RFC: Deprecate SapphireTest (was: Split unit testing support into silverstripe/test-support) #9668 is fine. The framework and each module can use the legacy testing approach, with the --ignore-platform-reqs flag. The change management issues you describe then don't come up. SapphireTest remains as a legacy but not-formally-deprecated feature.
• If your goal is to drop it from the core builds themselves, I'd ask "why?" sure, it's nice, but I'm not sure it's worth forking PHPUnit's module tree for. The benefits of upgrading to PHPUnit 9 should be more about the benefits of getting newer testing tools, rather than solely about PHP8 support. I'd also probably leave this until after we'd had some success about using PHPUnit 9 on projects. If you want, you could do a test composer install --no-dev in the CI build of the recipe to confirm that there are no package conflicts in the non-dev dependency (but not actually run any tests)

TL;DR: My advice would be proceed with #9668 with a focus on the testing of individual projects (or modules that don't form part of a kitchen-sink test) initially, and potentially leave it at that.

[emteknetnz]

I'm on board with your thinking on the other issue. I'll explain my perspective as a product dev a little better

The framework and each module can use the legacy testing approach, with the --ignore-platform-reqs flag

I don't think the --ignore-platform-reqs flag is a healthy long-term solution. This effectively means that product devs must use the --ignore-platform-reqs flag to get things to actually install on their local environment, and we need to do the same on travis-ci.com

While this may work for now in the isolated use-case being able to install phpunit 5.7 on php8, it will hide other non-phpunit dependencies that don't match the version of PHP being used.

This issue lists what currently gets installed on php8 when you use the --ignore-platform-reqs flag even though it is not compatible - namely totp, webauthn, elemental, behat and phunit. This is the same kind of result that we'd get as installing with --no-dev.

As we move into php 8.1 and beyond, we could easily run into a situation where a modules have dependencies that requires php 8.1, though we mistakenly assume we can install it on php 8.0, and then we tag a release that can't actually be installed.

I mentioned on the other issue that I consider forking ^5.7 phpunit to be technical debt. I also consider --ignore-platform-reqs to be technical debt, probably more so. Sure it's a very cheap option short term making it quite attracitve, though long term this is just delaying what needs to be done.

Moving forward

My goal here is to get core things to "just work" on php8. I see --ignore-platform-reqs as a short-term bridge though not a long-term solution.

class SapphireTest extends PHPUnit_Framework_TestCase

There's no way this will ever be PHP8 compatible without forking. There is no real appetite to keep these forks long term. So SapphireTest itself is also a blocker to PHP8 adoption, so as you recommend on the other issue it needs to be deprecated

Things won't "just work" on php8 until all the modules require phpunit ^9.3 - we need to update all the things. I don't think there's away around this, it's just how it is.

[sminnee]

I guess I'm making the argument that forking the entire dependency chain of phpunit 5.7 is just as problematic as continuing to use --ignore-platform-reqs. Why put a bunch of work into replacing one piece of awkward technical debt with another?

class SapphireTest extends PHPUnit_Framework_TestCase
There's no way this will ever be PHP8 compatible without forking

SapphireTest will forever be a phpunit5.7 support tool. Moving away from phpunit5.7 means moving away from SapphireTest. That's detailed in the other ticket.

as a short-term bridge though not a long-term solution

Could we agree that 'short-term' is, e.g. a year? Then we can get some real experience with project code (or non-kitchen-sink modules) freeing themselves of SapphireTest and using phpunit 9 for their own tests.

That way we could judge at that time whether it's better to migrate the framework and other tests away from SapphireTest, or keep propping up SapphireTest with a more complete phpunit 5.7 fork. FWIW I would probably take a "monorepo" approach if we were to do that, and inline the code of all the errant dependencies into sminnee/phpunit, rather than maintain dozens of separately forked repos.

It also means that we start by adding some value for project developers - the ability to move to phpunit 9 or other testing frameworks of their choice.

As we move into php 8.1 and beyond

Giving ourselves until Christmas 2021 ("a year" minus a bit) fits with that deadline, more or less.

[emteknetnz]

We could certainly make short-term one-year. As a rule I do all my local dev in the lowest supported version of php which is currently 7.1, so day to day this doesn't block me. It does however increase the risk though that we'll get something wrong in terms of compatible requirements.

My main concern driving all this is really that we'll prematurely announce "Silverstripe development works great with PHP8!", which may not be entirely accurate, or at least aggravates someone who heard the announcement but didn't read the nuanced release notes. I'm think we should be conservative with setting expectations otherwise we may be setting ourselves up for failure.

Waiting one year, in my mind at least, goes against us confidently telling people they should switch their project to PHP8.

It we can jettison SapphireTest for projects sooner rather than later I'm feeling more confident in PHP8 support.

What's stopping bespoke developers from using something other that SapphireTest now, or even 2 years ago? Are projects forced to unit test with SapphireTest due to some technical reasons?

phpunit.xml.dist in core modules has <phpunit bootstrap="vendor/silverstripe/framework/tests/bootstrap.php">

So if you kept this bootstrap.php in place, could you just write phpunit ^9 compatible unit test that extends TestCase? Or does SapphireTest trigger something that wouldn't happen if you only used TestCase?

[Cheddam]

SapphireTest provides DB fixture functionality, does a bunch of bootstrapping in setUp / tearDown, and provides a range of custom assertions. There's also FunctionalTest, which extends SapphireTest to provide helpers for testing web requests.

We'll need to replicate most of this API surface for developers to be able (re)write their tests using TestCase without loss of functionality.

[Cheddam]

If you want, you could do a test composer install --no-dev in the CI build of the recipe to confirm that there are no package conflicts in the non-dev dependency (but not actually run any tests)

Out of curiosity I just tried this locally, and got the following (somewhat passive aggressive) response from Composer:

Running update with --no-dev does not mean require-dev is ignored, it just means the packages will not be installed. If dev requirements are blocking the update you have to resolve those problems.

So we'll need to explicitly remove the phpunit dev requirements before running the composer install check.

[sminnee]

Are projects forced to unit test with SapphireTest due to some technical reasons?

There's a massive amount of state management that currently only exists in SapphireTest. My recommendation in the other ticket is to refactor that to the test-state services, so that SapphireTest becomes a few trivial lines.

[sminnee]

My main concern driving all this is really that we'll prematurely announce "Silverstripe development works great with PHP8!", which may not be entirely accurate, or at least aggravates someone who heard the announcement but didn't read the nuanced release notes. I'm think we should be conservative with setting expectations otherwise we may be setting ourselves up for failure.

Fair call; we probably want to frame this as "Experimental support for PHP8" until --ignore-platform-reqs is removed from the build.

[emteknetnz]

Closing as we're shipping PHPUnit 9.5 support with 4.10