G'day:
Today's plan
I sat down to write an article and some code using the spatie/async library. Not for any particular requirement I have, just that it sounds interesting. But I've just shelved that idea for today.
Today's revised plan
I added the package as a dependency and ran composer update. My first exercise after doing a composer update is always to run my automated tests to see what library updates have broken things. I'm glad I did because today it's PHPUnit itself that's breaking shit. Great.
So I backed-out spatie/async as a dependency, re-updated, and ran my tests again: yeah I still have problems. I checked and PHPUnit has updated from 9.5.28 to 9.6.2. I hasten to add that the breakages don't seem to be bugs or problems, just that they've added a bunch of deprecation notices in for upcoming changes to PHPUnit 10. This is a good thing. But I feel that now that I know about them, I should look into what I need to do to fix them. And I also suspect there's a way to run PHP with deprecation warnings switched off, and I should know how to do this so I am going to find out.
The symptoms I am seeing are this sort of thing:
Warning: Abstract test case classes with "Test" suffix are deprecated … ┐ ├ assertObjectHasAttribute() is deprecated and will be removed in PHPUnit 10. ┴ … ┐ ├ Expecting E_ERROR and E_USER_ERROR is deprecated and will no longer be possible in PHPUnit 10. ┴ …
Those seem like the only three actually. Let's find out how to work around them.
Not PHPUnit's finest hour
First: how to suppress the warnings. This is not helped by the phpunit.xml page in the docs currently 404ing (this is a temporary thing I think: it was fine last week, I have it in my history). But I found a cached version. Having done some googling, there is no way of turning off the deprecation reporting it seems (it was requested, and actively rejected). I also found this article written by Sebastian Bergmann, PHP's lead: Help! My tests stopped working ("Deprecations" section). Relevant extract:
PHPUnit behaves differently compared to PHP and how it reports the usage of deprecated functionality. When a deprecated feature of PHP is used then a message of type E_DEPRECATED is created. This message, however, is only shown when the developer has configured PHP's error_reporting setting accordingly. In my experience, developers either do not have error_reporting configured correctly (it should be set to -1 for development so that you see all error messages) or never bother to look at error messages that get buried in some log file. This is why quite a few developers are surprised that their software stops working when a major version of PHP is released that removes previously deprecated functionality.
PHPUnit reports a test that uses deprecated functionality with a warning because I know how developers use, or rather not use, PHP's E_DEPRECATED messages. You cannot opt out of getting this information from PHPUnit.
IMO this is a bit of a dick move (even if the intent is sound). PHPUnit: your job? Being a unit test framework. Stick to that. Leave it to me and PHP how deprecations ought to be dealt with. That's not part of your remit. If there was no prescribed way of handling (or not ~) in PHP then fair enough for PHPUnit to make its own call. And if PHPUnit was unaware of the prescribed PHP approach: also fair enough. But to be aware of it and pronounce "well ackchyually… I know better" is letting the side down a bit. IMO, obvs.
Still: it's given me a blog article topic I guess. So… erm…"cheers"…?
Firstly: the article also says this:
By default, PHPUnit's command-line test runner exits with shell exit code 0 when the use of a deprecated feature is reported.
I'm staring at an exit code of 1, currently, but I am guessing this is because I have failOnWarning="true" set in phpunit.xml.dist.
WARNINGS! Tests: 79, Assertions: 208, Warnings: 8, Skipped: 1. Generating code coverage report in HTML format ... done [00:06.012] root@19f76723847c:/var/www# echo $? 1
Yeah, this returns zero if I switch that setting off. It still sux though, cos I don't want to turn warnings off, I only want to not be pestered by PHPUnit's own deprecations interfering with the reporting of the tests of my own code, which - actually - are all fine. So this is a case of misreporting on PHPUnit's part. To me that's a bug (one implemented by design, but subpar-decision-making is as much a bug as shonky implementation is).
NB: the quickest fix for me here would be to roll back to the previous version of PHPUnit whilst I take time to scheduling in the fixing of the tests. I hasten to add I am completely fine making the code changes, that's not the issue here. It's the crap way it's being handled by PHPUnit is the only issue.
Anyway, we get it: I'm annoyed. Let's move on and do something useful.
Abstract test case classes with "Test" suffix are deprecated
My initial reaction to this is about as positive as my reaction to PHPUnit's deprecation handling. It's completely legit for test suites to have common notionality in an abstract sense. And the best description of this could very legitimately end in the word Test. An obvious example might be a polypmorhic sitution with the classes under test, which could have different imlpementations each with the same custom assertion needs. Or even the implementation of dependencies to the object under test might be the same. On the other hand, from a position of not having looked into why PHPUnit has had this change make, there seems no good reason to prevent this: this seems like meddling for the sake of it to me, but let's see if I can find out why they've done it. It would be great, btw, if these deprecation notices also linked to whatever issue caused the deprecation to be made. But I'll hunt through the code.
The first thing I have found is the Github issue that represents the work: no explanation. No text at all, which is less than ideal: no work should be undertaken with a description of what/why, and also like some acceptance criteria so we know what the "definition of done" is for the given case. The only other thing I found is another Github issue of someone being bitten on the bum by it, but there was only confirmation it's "expected" behaviour, but no explanation why. Oh well.
This is at least easy to fix. I can simply rename the class in question.
In the case in question it's just this:
abstract class HttpTest extends TestCase
{
protected function getUserAgentForCurl(): string
{
return sprintf("curl/%s", curl_version()['version']);
}
protected function assertGitInfoIsCorrect(string $response): void
{
$myGitMetadata = json_decode($response);
$this->assertEquals('adamcameron', $myGitMetadata->login);
$this->assertEquals('Adam Cameron', $myGitMetadata->name);
}
}
TBH hoisting the getUserAgentForCurl helper method does not really contribute to a sense that there's an is-a relationship between this class and the subclasses (CurTest, GuzzleTest, PhPStreamTest, SymfonyHttpClientTest): this would better be in a dependency class if that's all that was in play here. But I def think the common need for the same custom assertion implementation fits the bill, so the abstractness of this class and its subclass hierarchy is legit. I also think HttpTest is the most fitting name for it, but… oh well. I'm just gonna rename it to HttpTestBase (runner up: HttpTests (plural)) I think. Not great - and not as fitting as HttpTest as it is now - but so be it.
assertObjectHasAttribute() is deprecated and will be removed in PHPUnit 10.
I found the issue for this one straight away: Remove assertions that operate on class/object properties #4601. This situation is legit. Quoting the ticket:
PHPUnit currently refers to "fields" (see above) as "attributes". This is (or will become) confusing considering the introduction of attributes in PHP 8 and their support in PHPUnit.
PHPUnit will be changed to use the term "property" instead of "attribute" where "field" is meant.
I'm onboard with this: it makes sense.
But. The replacement methods have not been implemented yet. So this is a good example where this thing oh PHPUnit's of raising deprecations as warnings is shit. I can't switch this one off "for now", knowing I have some work to do in future. I also don't want to turn off the PHPUnit setting that promotes warnings to exceptions because I want to know about legitimate warnings (from other sources). This is why PHP handles deprecations the way that it does. Sigh.
I want to get rid of the warnings - well I have to! - So I'm gonna implement my own assertObjectHasProperty assertion, using the same implementation as the current assertObjectHasAttribute. It's only used in one test class, so it's easy to integrate it into my codebase as well.
[beat]
OK so not exactly the same implementation as the current one uses private methods from its current home /src/Framework/Assert.php. But I'll knock together as close an approximation as possible I guess.
[beat]
OK screw that: it's using other deprecated methods on other objects, and it's not a good use of my time reimplementing all this crap. I'm just gonna use a property_exists check inline where I am currently using assertObjectHasAttribute. There's only two occurrences.
Expecting E_ERROR and E_USER_ERROR is deprecated and will no longer be possible in PHPUnit 10.
This is another case of the deprecation being put in with no real explanation. It's issue #5062, and the "explanation" is:
There are no replacements. PHPUnit 10 no longer converts E_* to exceptions, therefore E_* can no longer be expected.
Fine, but throw us a bone, pal: why does "PHPUnit 10 no longer converts E_* to exceptions, therefore E_* can no longer be expected". There must be some rationale? Let's go see if I can find anything about that… … … Nope. No issue I can find relating to that. All right (shakes head, and mutters "fuck sake, PHPUnit").
Right so I've addressed this one thus:
Before:
/** @testdox It cannot be type-coerced */
public function testTypeCoercion()
{
$this->expectError(); // NB: not an exception; an error
$this->expectErrorMessageMatches("/.*MaoriNumbers could not be converted to int.*/");
$this->assertEquals(sprintf("ono: %d", MI::ONO), "ono: 6");
}
After:
/** @testdox It cannot be type-coerced */
public function testTypeCoercion()
{
$this->assertError(
function () {
$this->assertEquals(sprintf("ono: %d", MI::ONO), "ono: 6");
},
"MaoriNumbers could not be converted to int"
);
}
public function assertError(callable $callback, string $message)
{
try {
$callback();
$this->fail("Expected an error, didn't get one");
} catch (AssertionFailedError $e) {
throw $e;
} catch (\Throwable $e) {
$this->assertStringContainsString($message, $e->getMessage());
}
}
This is not perfect, but it'll do for what I'm needing it for. I have a coupla instances of this I need to refactor, and both in the same package. So I'll chuck this custom assertion in a trait I guess. One of the few times I don't feel icky using traits.
Conclusion
This was not a very edifying exercise, and I don't really think anything's been improved by two out of three of these PHPUnit changes I've needed to work around. Still: it's not the end of the world, and I hasten to add for my gripes here, I still think PHPUnit is brilliant. Mostly ;-) (kidding, it's great).
I guess I'll finally get to write the article I wanted to write tomorrow evening.
The current state of this project is tagged as 1.14.
Righto.
--
Adam
