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

Polyfill for equalTo constraint specializations #28

Merged
merged 7 commits into from
Jun 14, 2021
Merged

Polyfill for equalTo constraint specializations #28

merged 7 commits into from
Jun 14, 2021

Conversation

mergeMarc
Copy link
Contributor

@mergeMarc mergeMarc commented Jun 10, 2021

This PR adds the EqualToSpecializations Trait that adds polyfills for the in PHPUnit 9.0.0 introduced methods equalToCanonicalizing, equalToIgnoringCase and equalToWithDelta.

Since PHPUnit Version 9.0.0 equalTo constraints got their own specializations to replicate the assertEquals specializations. (see sebastianbergmann/phpunit@43c01a4) The usage of equalTo with these parameters never got deprecated and also had no replacement until the functionality was removed (see sebastianbergmann/phpunit@e470a3a).
This means that test cases that want to use equalTo with additional configuration cannot be written to work with both PHPUnit 8 (or older) and PHPUnit 9 without the use of a polyfill.

@mergeMarc mergeMarc marked this pull request as ready for review June 10, 2021 17:13
@jrfnl
Copy link
Collaborator

jrfnl commented Jun 10, 2021

@mergeMarc Thanks for this PR.

For my understanding: is there a particular reason why you are using the equalToCanonicalizing(), equalToIgnoringCase() and equalToWithDelta() methods directly and not using the assert[Not]EqualsCanonicalizing(), assert[Not]EqualsIgnoringCase() assert[Not]EqualsWithDelta() methods ?

The assertion methods are already polyfilled in the Yoast\PHPUnitPolyfills\Polyfills\AssertEqualsSpecializations trait and have been from the start (initial release) of this library.

Note: I'm not adverse to including this, I'd just like to understand the usecase.

@mergeMarc
Copy link
Contributor Author

I totally understand. I didn't find other people mentioning this issue anywhere so usage is probably not so wide spread.

I need to use it with PHPUnit Mock Objects. To check parameters that are passed to a mocked method call one needs to use a Constraint instead of the usual asserting of a value right away (see docs example).

Example:

In this excerpt of a simple test case I check if my main application uses my "PersistenceInterface" correctly. I need to include the delta here since the DateTime value of my "Day" will always be off by a few milliseconds. If I run the first version in PHPUnit 9 the test will always fail since the delta is not considered (equalTo only accepts one parameter since v9).

PHPUnit < 9

$today = new Day($todayDate, $todaysWeather);
$storage = $this->createMock(PersistenceInterface::class);
$storage->expects($this->once())
            ->method('updateDay')
            ->with($this->equalTo($today, 10));
...

PHPUnit >= 9

$today = new Day($todayDate, $todaysWeather);
$storage = $this->createMock(PersistenceInterface::class);
$storage->expects($this->once())
            ->method('updateDay')
            ->with($this->equalToWithDelta($today, 10));
...

@jrfnl
Copy link
Collaborator

jrfnl commented Jun 11, 2021

@mergeMarc Thanks for that additional information. Makes sense to me and I'm happy to include it.

There are a few more (minor) changes needed for this PR to be complete. I'd be happy to make those or give you feedback to allow you make them. What would you prefer ?

@jrfnl jrfnl added this to the 0.x Next Release milestone Jun 11, 2021
@mergeMarc
Copy link
Contributor Author

@jrfnl Feel free to commit those changes. I would also be available to make them if they require more work. Thank you.

@jrfnl
Copy link
Collaborator

jrfnl commented Jun 11, 2021

@mergeMarc No, nothing major. I've just pushed the commit and will give you a moment to look it over.

One of us will still need to rebase the PR to make it mergable.

@jrfnl
Copy link
Collaborator

jrfnl commented Jun 11, 2021

Ended up adding two more touch up commits. Will stop looking now ;-) (and will squash on merge or rebase).

@jrfnl
Copy link
Collaborator

jrfnl commented Jun 14, 2021

Rebased on develop to make the PR mergable. No new changes.

Copy link
Collaborator

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mergeMarc Thanks a lot for this PR. Great contribution and well done on the completeness of the PR. This new feature will be included in the next release.

@jrfnl jrfnl merged commit 6482b50 into Yoast:develop Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants