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

Convenient custom comparison of objects #4467

Closed
sebastianbergmann opened this issue Sep 23, 2020 · 6 comments
Closed

Convenient custom comparison of objects #4467

sebastianbergmann opened this issue Sep 23, 2020 · 6 comments
Assignees
Labels
feature/assertion Issues related to assertions and expectations type/enhancement A new idea that should be implemented
Milestone

Comments

@sebastianbergmann
Copy link
Owner

Suggested by @spriebsch:

It is a bad practice to use assertEquals() (and its inverse, assertNotEquals()) on objects without registering a custom comparator that customizes how objects are compared. Unfortunately, though, implementing custom comparators for each and every object you want to assert in your tests is inconvenient at best and overkill at worst.

The most common use case for custom comparators are Value Objects. These objects usually have an equals(self $other): bool method (or a method just like that but with a different name) for comparing two instances of the Value Object's type.

We propose a new assertion method, assertObjectEquals(), that makes custom comparison of objects convenient for this common use case:

public function assertObjectEquals(object $expected, object $actual, string $method = 'equals', string $message = '')
{
}
  • A method with name $method must exist on the $actual object
  • The method must accept exactly one argument
  • The respective parameter must have a declared type
  • The $expected object must be compatible with this declared type
  • The method must have a declared bool return type

If any of the aforementioned assumptions is not fulfilled or if $actual->$method($expected) returns false then the assertion fails.

An inverse of assertObjectEquals() does not make sense to us and will not be implemented.

Example

Email.php

<?php declare(strict_types=1);
final class Email
{
    private string $email;

    public function __construct(string $email)
    {
        $this->ensureIsValidEmail($email);

        $this->email = $email;
    }

    public function asString(): string
    {
        return $this->email;
    }

    public function equals(self $other): bool
    {
        return $this->asString() === $other->asString();
    }

    private function ensureIsValidEmail(string $email): void
    {
        // ...
    }
}

SomethingThatUsesEmailTest.php

<?php declare(strict_types=1);
use PHPUnit\Framework\TestCase;

final class SomethingThatUsesEmailTest extends TestCase
{
    public function testSomething(): void
    {
        $a = new Email('user@example.org');
        $b = new Email('user@example.org');
        $c = new Email('user@example.com');

        // This should pass
        $this->assertObjectEquals($a, $b);

        // This should fail
        $this->assertObjectEquals($a, $c);
    }
}
@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented feature/assertion Issues related to assertions and expectations labels Sep 23, 2020
@sebastianbergmann sebastianbergmann added this to the PHPUnit 9.4 milestone Sep 23, 2020
@sebastianbergmann sebastianbergmann self-assigned this Sep 23, 2020
@sebastianbergmann sebastianbergmann changed the title assertObjectEquals() Convenient custom comparison of objects Sep 23, 2020
@francescolaffi
Copy link

Hi @sebastianbergmann, it looks like this is the only non-static assertion, and I couldnt find any reason obvious to me.
Maybe there is some reason and it would be helpful to comment it in the code to explain the oddity, or maybe just an oversight. In any case let me know if you'd like a issue/MR for this. thanks!

@sebastianbergmann
Copy link
Owner Author

Thank you for bringing this oversight to my attention.

@3DFace
Copy link

3DFace commented Dec 14, 2020

Hi @sebastianbergmann!

Unfortunately, though, implementing custom comparators for each and every object you want to assert in your tests is inconvenient at best and overkill at worst.

I'm curious if it could be implemented as a single comparator, something like ByEqualsMethodObjectComparator.
Or as evolution of default ObjectComparator?

Otherwise how can we to check equality of two arrays of such objects now?

@webmozart
Copy link
Contributor

webmozart commented Apr 8, 2021

One downside here is that you lose the helpful exception message that tells you why exactly the comparison of the two objects fails.

For an alternative approach, I created a StrictPHPUnitExtension that enables strict comparisons of all scalar values during assertEquals(). With that extension enabled, you can continue to compare value objects with assertEquals(), but all scalar properties PHPUnit finds within those will be checked with type safety.

@spriebsch
Copy link

Thanks for pointing that out, Bernhard. I'll have a look at that extension. With regards to "losing the helpful exception message", I think that's just a matter of which details the failed assertion outputs: on "false" result of the object's equals method, it might indeed make sense to show a comparison of the scalar values to ease debugging.

However, I am a little concerned (and that's one of the reasons why I made that feature suggestion in the first place) that there might be situations where you have to different implementations of value objects representing the same thing, e.g. a GenericThing and a SpecificThing object, but both represent the same thing from a domain perspective, so I would expect "equals" to return true. In such a case, showing a "scalar values diff" would of course make no sense at all - but I do acknowledge that that is an edge case that should not keep anybody from using/showing a "scalar value diff" by default on failed equals comparison.

@webmozart
Copy link
Contributor

webmozart commented Apr 9, 2021

I completely agree Stefan, in such special cases, an equals() method in combination with assertObjectEquals() is the way to go.

It seems hard to me to facilitate debugging in the assertObjectEquals() method since we don't know based on what information equals() decides whether two objects are equal. In your example, pointing out that GenericThing is not a SpecificThing is misleading, since equals() actually considers the two to be the same. The same is true for differences in property values: Those might matter or they may not.

What does the PHPUnit core team think about progressively moving the ScalarComparator to strict comparisons by default? You could add a strict setting in the next major release that defaults to false and then flip the default to true in a later major release. In times of strict_types, Psalm and PHPStan, I doubt there are much projects left that actually need assertEquals() to compare without type safety.

jrfnl added a commit to Yoast/PHPUnit-Polyfills that referenced this issue Jun 17, 2021
…ethod

PHPUnit 9.4.0 introduced the new `Assert::assertObjectEquals()` method.

This commit:
* Adds two traits with the same name.
    One to polyfill the method when not available in PHPUnit.
    The other to allow for `use`-ing the trait in PHPUnit versions in which the method is already natively available.
* Adds an `InvalidComparisonMethodException` exception class.
    _PHPUnit natively throws a range of different exceptions._
    _The polyfill included in this library throws one exception type - the `InvalidComparisonMethodException` - with a range of different messages._
* Logic to the custom autoloader which will load the correct trait depending on the PHPUnit version used.
* Adds tests.

As the polyfill contains logic to match the PHPUnit native implementation as closely as possible, while still being PHP and PHPUnit cross-version compatible, extensive unit tests have been added to ensure the behaviour of the polyfill matches that of the original function, with the exception of the _return type verification_.

As return types were not available in PHP prior to PHP 7.0, the return type verification as done in the PHPUnit native implementation, has been replaced by a verification that the _returned value_ is of the required type.
This provides the same safeguard as the PHPUnit native implementation, but in a PHP cross-version compatible manner.

Note: the method uses `static::` to call the PHPUnit native functionality. This allows for existing method overloads in a child class of the PHPUnit native `TestCase` to be respected.

Includes:
* Adding information on the new polyfill to the README.
* Adding the new polyfill to the existing `TestCases` classes.
* Adding a few select exceptions to the PHPCS ruleset for the fixtures used in the tests.

Refs:
 * sebastianbergmann/phpunit#4467
 * sebastianbergmann/phpunit#4707
 * sebastianbergmann/phpunit@1dba8c3
 * sebastianbergmann/phpunit@6099c5e
jrfnl added a commit to Yoast/PHPUnit-Polyfills that referenced this issue Jun 17, 2021
…ethod

PHPUnit 9.4.0 introduced the new `Assert::assertObjectEquals()` method.

This commit:
* Adds two traits with the same name.
    One to polyfill the method when not available in PHPUnit.
    The other to allow for `use`-ing the trait in PHPUnit versions in which the method is already natively available.
* Adds an `InvalidComparisonMethodException` exception class.
    _PHPUnit natively throws a range of different exceptions._
    _The polyfill included in this library throws one exception type - the `InvalidComparisonMethodException` - with a range of different messages._
* Logic to the custom autoloader which will load the correct trait depending on the PHPUnit version used.
* Adds tests.

As the polyfill contains logic to match the PHPUnit native implementation as closely as possible, while still being PHP and PHPUnit cross-version compatible, extensive unit tests have been added to ensure the behaviour of the polyfill matches that of the original function, with the exception of the _return type verification_.

As return types were not available in PHP prior to PHP 7.0, the return type verification as done in the PHPUnit native implementation, has been replaced by a verification that the _returned value_ is of the required type.
This provides the same safeguard as the PHPUnit native implementation, but in a PHP cross-version compatible manner.

Note: the method uses `static::` to call the PHPUnit native functionality. This allows for existing method overloads in a child class of the PHPUnit native `TestCase` to be respected.

Includes:
* Adding information on the new polyfill to the README.
* Adding the new polyfill to the existing `TestCases` classes.
* Adding a few select exceptions to the PHPCS ruleset for the fixtures used in the tests.

Refs:
 * sebastianbergmann/phpunit#4467
 * sebastianbergmann/phpunit#4707
 * sebastianbergmann/phpunit@1dba8c3
 * sebastianbergmann/phpunit@6099c5e
jrfnl added a commit to Yoast/PHPUnit-Polyfills that referenced this issue Jun 17, 2021
…ethod

PHPUnit 9.4.0 introduced the new `Assert::assertObjectEquals()` method.

This commit:
* Adds two traits with the same name.
    One to polyfill the method when not available in PHPUnit.
    The other to allow for `use`-ing the trait in PHPUnit versions in which the method is already natively available.
* Adds an `InvalidComparisonMethodException` exception class.
    _PHPUnit natively throws a range of different exceptions._
    _The polyfill included in this library throws one exception type - the `InvalidComparisonMethodException` - with a range of different messages._
* Logic to the custom autoloader which will load the correct trait depending on the PHPUnit version used.
* Adds tests.

As the polyfill contains logic to match the PHPUnit native implementation as closely as possible, while still being PHP and PHPUnit cross-version compatible, extensive unit tests have been added to ensure the behaviour of the polyfill matches that of the original function, with the exception of the _return type verification_.

As return types were not available in PHP prior to PHP 7.0, the return type verification as done in the PHPUnit native implementation, has been replaced by a verification that the _returned value_ is of the required type.
This provides the same safeguard as the PHPUnit native implementation, but in a PHP cross-version compatible manner.

Note: the method uses `static::` to call the PHPUnit native functionality. This allows for existing method overloads in a child class of the PHPUnit native `TestCase` to be respected.

Includes:
* Adding information on the new polyfill to the README.
* Adding the new polyfill to the existing `TestCases` classes.
* Adding a few select exceptions to the PHPCS ruleset for the fixtures used in the tests.

Refs:
 * sebastianbergmann/phpunit#4467
 * sebastianbergmann/phpunit#4707
 * sebastianbergmann/phpunit@1dba8c3
 * sebastianbergmann/phpunit@6099c5e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/assertion Issues related to assertions and expectations type/enhancement A new idea that should be implemented
Projects
None yet
Development

No branches or pull requests

5 participants