Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update tests for PHPUnit 9 #2936

Closed
elazar opened this issue Apr 3, 2020 · 13 comments · Fixed by #2938
Closed

Update tests for PHPUnit 9 #2936

elazar opened this issue Apr 3, 2020 · 13 comments · Fixed by #2938
Labels

Comments

@elazar
Copy link
Contributor

elazar commented Apr 3, 2020

Two deprecation notices are emitted when running the current unit test suite under PHPUnit 8.5.3.

1) Slim\Tests\MiddlewareDispatcherTest::testResolveThrowsExceptionWithoutContainerAndUnresolvableClass
expectExceptionMessageRegExp() is deprecated in PHPUnit 8 and will be removed in PHPUnit 9.

2) Slim\Tests\MiddlewareDispatcherTest::testResolveThrowsExceptionWithoutContainerNonAdvancedCallableResolverAndUnresolvableClass
expectExceptionMessageRegExp() is deprecated in PHPUnit 8 and will be removed in PHPUnit 9.

It doesn't appear that there's an intended replacement for the deprecated method.

There are two potential approaches to handle this that I can think of:

  1. Wrap the method call that results in the exception being thrown in a try block and handling the regular expression match check in the corresponding catch block using a different assertion method (e.g. assertSame(1, preg_match('...', $exception->getMessage()))).
  2. Switch to using expectExceptionMessage(), which checks that the exception message contains a specified substring, and have it check for the common part of the substring expressed by the existing regular expression.

Thoughts? I'd be happy to submit a PR for this myself if I know the preferred approach.

Updated: Currently blocked by sebastianbergmann/phpunit#4142, sebastianbergmann/phpunit#4149.

@l0gicgate
Copy link
Member

We should switch to expectedExceptionMessage()

@adriansuter
Copy link
Contributor

Absolutely.

@elazar
Copy link
Contributor Author

elazar commented Apr 3, 2020

I've filed #2938 to address this, but it currently spams the console with deprecation warnings for PHPUnit 10 when run under PHP 7.3 and 7.4, particularly this one.

PHPUnit\Framework\TestCase::prophesize() is deprecated and will be removed in PHPUnit 10. Please use the trait provided by phpspec/prophecy-phpunit.

Unfortunately, the cited package made the change to add the referenced trait fairly recently -- see phpspec/prophecy-phpunit#20 -- and it doesn't appear to be included in a tagged release yet.

Depending on your thoughts on this, you may want to hold off on merging this change until the situation is resolved.

@l0gicgate
Copy link
Member

@elazar that means we would need to add ProphecyTrait to all of the test cases using this correct? I'm fine with doing that.

@elazar
Copy link
Contributor Author

elazar commented Apr 4, 2020

@l0gicgate The issue is that the trait isn't yet available in a tagged release of the phpspec/prophecy-phpunit package; it's only available on the master branch. In other words, we'd need to set the version of that dependency to dev-master in composer.json for the trait to be available, which I'm assuming is something we don't want to do for reasons pertaining to build stability.

@l0gicgate
Copy link
Member

We should probably hold off for now on this then. It's not pressing, we can migrate to the trait once they have a stable release out @elazar!

@elazar
Copy link
Contributor Author

elazar commented Apr 4, 2020

@l0gicgate Understood. I'll keep an eye out for that release. Once it's available, I'll update this PR accordingly.

@t0mmy742
Copy link
Contributor

expectExceptionMessageRegExp() should be replaced by expectExceptionMessageMatches(), not expectedExceptionMessage() (sebastianbergmann/phpunit#3957)

assertRegExp() also changed to become assertMatchesRegularExpression() (sebastianbergmann/phpunit#4086)

PHPUnit 10 requires phpspec/prophecy-phpunit ^2.0 to have ProphecyTrait (PHPUnit does not provide prophecy anymore). A new release has been tagged. BUT it requires PHP ^7.3. We can not use this package while Slim use PHP ^7.2.

This commit t0mmy742/Slim@82c37f2 provides the modifications (if needed, PR can be made).
Of course we can stay with PHPUnit 8.5 (PHPUnit requires PHP ^7.3). Note that PHP 7.2 is not in active support since 30 Nov 2019

@elazar
Copy link
Contributor Author

elazar commented Apr 11, 2020

@t0mmy742 Thanks for the notes on the assertion method changes.

A new release of phpspec/prophecy-phpunit has been tagged, but a new release of PHPUnit 9 that references this new release of phpspec/prophecy-phpunit has not yet been tagged.

And, as you pointed out, PHPUnit >= 9 requires PHP 7.3. I actually didn't realize that Slim was only requiring 7.2, and agree that there should probably be a bump to 7.3 or 7.4 in Slim 5. (I'm assuming any sooner would violate semver?)

@t0mmy742
Copy link
Contributor

@elazar PHPUnit is not going to reference phpspec/prophecy-phpunit in his composer.json. Now, if you need prophecy, you need to require manually phpspec/prophecy AND phpspec/prophecy-phpunit (that's what I understood).

@elazar
Copy link
Contributor Author

elazar commented Apr 11, 2020

@t0mmy742 Ah. I suppose that does make sense.

@l0gicgate @adriansuter Any thoughts on this thread's latest discussion?

@l0gicgate
Copy link
Member

@elazar we will need to wait until PHP 7.2 security support ends to implement this (Nov 30th, 2020). We cannot do a version bump to 7.3 until then.

Slim 5 development isn't in the cards yet, so waiting for a major Slim version bump is going to take a while.

@elazar
Copy link
Contributor Author

elazar commented Apr 13, 2020

@l0gicgate Understood. For now, I've updated #2938 to not use PHPUnit 9 and to instead just replace the deprecated methods with the new equivalents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants