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

Make export of objects customizable #5758

Open
BladeMF opened this issue Mar 20, 2024 · 24 comments
Open

Make export of objects customizable #5758

BladeMF opened this issue Mar 20, 2024 · 24 comments
Labels
feature/test-runner CLI test runner type/enhancement A new idea that should be implemented

Comments

@BladeMF
Copy link

BladeMF commented Mar 20, 2024

The Exporter that provides text representation of arguments and assertions. There are, however, really big objects which are not practical to be displayed (e.g. Doctrine entities, Phpstan test case arguments). Since the actual Exporter class is not injected and is not an interface, its behaviour cannot be modified. If I have a test that is asserting equality between two Doctrine entities and it fails, the displayed error is hundreds of screens long and not practical.

It should be possible to decorate the default export and modify its behaviour without that affecting anything in Phpunit. That way one can create plugins that print certain large objects properly - doctrine and phpstan included.

@BladeMF BladeMF added the type/enhancement A new idea that should be implemented label Mar 20, 2024
@sebastianbergmann
Copy link
Owner

Configurable on the test suite level (in the XML configuration file) or on the test level (using an attribute)?

@BladeMF
Copy link
Author

BladeMF commented Mar 20, 2024

Ideally it should be on project level, e.g. install a plugin to phpunit.

@BladeMF
Copy link
Author

BladeMF commented Mar 20, 2024

My current problems, to be precise are these:

  1. I cannot compare Doctirne entities, because if the test fails, their textual representation is huge and unreadable.
  2. The test cases for my Phpstan extension, mentioned here are now taking 5 minutes or longer, because of the exporting of the arguments of the data providers before the test runs.

@ondrejmirtes
Copy link

Similar problem that happened previously: #5524

@sebastianbergmann
Copy link
Owner

Correct me if I'm wrong, but what you are asking for can already be achieved using a custom comparator:

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

final class C
{
    private array $data;
    
    public function __construct(array $data)
    {
        $this->data = $data;
    }

    public function largeDataStructure(): array
    {
        return $this->data;
    }
}

final class Test extends TestCase
{
    public function testOne(): void
    {
        $this->registerComparator(
            new class extends Comparator
            {
                public function accepts(mixed $expected, mixed $actual): bool
                {
                    return true;
                }

                public function assertEquals(mixed $expected, mixed $actual, float $delta = 0.0, bool $canonicalize = false, bool $ignoreCase = false): void
                {
                    $expectedAsString = 'foo';
                    $actualAsString   = 'bar';

                    throw new ComparisonFailure(
                        $expected,
                        $actual,
                        $expectedAsString,
                        $actualAsString,
                        'Failed asserting that ...',
                    );
                }
            }
        );

        $a = new C([1, 2, 3]);
        $b = new C([4, 5, 6]);

        $this->assertEquals($a, $b);
    }
}

Of course, you can also "simplify" $expected and $actual before passing them to ComparisonFailure (from where PHPUnit will pass them to Exporter, IIRC).

@BladeMF
Copy link
Author

BladeMF commented Mar 20, 2024

Correct me if I'm wrong, but what you are asking for can already be achieved using a custom comparator:

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

final class C
{
    private array $data;
    
    public function __construct(array $data)
    {
        $this->data = $data;
    }

    public function largeDataStructure(): array
    {
        return $this->data;
    }
}

final class Test extends TestCase
{
    public function testOne(): void
    {
        $this->registerComparator(
            new class extends Comparator
            {
                public function accepts(mixed $expected, mixed $actual): bool
                {
                    return true;
                }

                public function assertEquals(mixed $expected, mixed $actual, float $delta = 0.0, bool $canonicalize = false, bool $ignoreCase = false): void
                {
                    $expectedAsString = 'foo';
                    $actualAsString   = 'bar';

                    throw new ComparisonFailure(
                        $expected,
                        $actual,
                        $expectedAsString,
                        $actualAsString,
                        'Failed asserting that ...',
                    );
                }
            }
        );

        $a = new C([1, 2, 3]);
        $b = new C([4, 5, 6]);

        $this->assertEquals($a, $b);
    }
}

Of course, you can also "simplify" $expected and $actual before passing them to ComparisonFailure (from where PHPUnit will pass them to Exporter, IIRC).

Actually I think I have considered that solution (I've looked extensively through the source code since 9.5), but found something lacking.

What if I wan't to compare, throughout my project, complex data structures (arrays, objects, etc.) which might or might not contain things I'd want to simplify on various levels thoughout the given structure. This means I need to a) recreate the original equality comparer from scratch, or b) somehow use it for everything but the complicated objects, which gets incredibly difficult. Am I making any sense to you?

@sebastianbergmann
Copy link
Owner

Am I making any sense to you?

Not really, sorry.

Maybe this helps: registerComparator() only registers an additional comparator, it does not replace any of the existing ones. assertEquals() will try the registered comparators, custom and built-in ones, one after the other. The first one that says "I can compare this!" (by returning true from accepts() will be used to perform the assertion (by calling its assertEquals() method).

@BladeMF
Copy link
Author

BladeMF commented Mar 20, 2024

I get your point. Lets consider an example.

Say that I want a custom string representation of the class Apple, because the default one is too big. Then, I can register a comparator that expects two instances of the Apple class. So far, so good. This means that this code is fine and dandy:

$a = new Apple(3);
$b = new Apple(5);
self::assertEquals($a, $b);

Imagine then I have this code:

self::assertEquals(
    [
        'key1' => 10,
        'key2' => new Apple(5),
        'key3' => [,
             'key3.1' => 'string 1',
             'key3.2' => new Apple(7),
        ],
    ],
    getDataStructure(),
);

Will then my comparer be called for keys key2 and key3.2?

@sebastianbergmann
Copy link
Owner

No.

@BladeMF
Copy link
Author

BladeMF commented Mar 20, 2024

See my point? If I compare data structures that contain the objects I need to use my comparer on, it won't be called and the default textual representation is used. So using a custom comparer has a very limited application even if it is a workaround in these cases.

But this gets us back to the original point - we don't really care about the comparing. What we care about is the textual representation.

P.S. It would be nice to be able to have a custom comparer that works in all cases though. Say, for example, if the equality comparer works on type level - for every value compared get the relevant comparer and when there are containers (arrays/colletions of values or objects with properties), the relevant container is called for each property/key type. But that's a different subject.

P.P.S. I'd be happy to contribute when time permits if you're open to the idea we're discussing.

@sebastianbergmann
Copy link
Owner

This is a complex topic that I am uncomfortable with delegating and I will look into this when I find the time.

@BladeMF
Copy link
Author

BladeMF commented Mar 21, 2024

I completely understand that. Given that you probably don't have enough time, what if I told you it would be completely okay if you decided to completely throw away my work at the end? I can explain my idea of doing it, probably do some prototyping and I won't mind if you decide my work sucks at the end. (Imagine how bad I need this 🤣😂🤣)

@staabm
Copy link
Contributor

staabm commented Mar 28, 2024

as I stumbled over this today when running the test-suite of roave/BetterReflection:

Exporter->resursiveExporter consumes a huge portion of the required memory and the same is true for CPU time:

grafik

grafik

@sebastianbergmann
Copy link
Owner

I know, @staabm. It is not easy to tackle, though.

As far as I understand it, though, the most problematic Exporter usage we have is that for data coming from a data provider into a test. And this is best addressed outside of PHPUnit by not generating large object graphs that are expensive to export inside data providers in the first place. IMO, data providers should, especially when the resulting object graphs are expensive, provide scalar input to the test method where that input is used to create the expensive data structures.

That being said, I am willing to make Exporter configurable, but improving performance is not my primary motivation for that. It's a nice bonus for sure, though.

@staabm
Copy link
Contributor

staabm commented Mar 28, 2024

As far as I understand it, though, the most problematic Exporter usage we have is that for data coming from a data provider into a test.

I also think thats the main problem. is this data only used for showing it to the human looking at the phpunit result output?
if so, we could think about e.g. limiting the depth how far this data is exported, because a deeply nested structure is also not very useful for a human sitting infront of the screen

@sebastianbergmann sebastianbergmann added the feature/test-runner CLI test runner label Mar 28, 2024
@sebastianbergmann
Copy link
Owner

I am working on a proof-of-concept that may already be "good enough", hope to have it in a shareable state soon.

@ondrejmirtes
Copy link

At least I fixed the performance problem in PHPStan by passing around plain strings instead: phpstan/phpstan-src@da87a65

@sebastianbergmann sebastianbergmann linked a pull request Mar 28, 2024 that will close this issue
8 tasks
@sebastianbergmann
Copy link
Owner

I think this could be as simple as #5773.

@staabm
Copy link
Contributor

staabm commented Mar 29, 2024

@BladeMF was the primary motivation for this issue really that you want to implement exporter methods for certain objects or is your actual primary concern only in the inefficient performance of the out-of-the-box export?

I am wondering whether we really need the configurable export, or whether just a more efficient export would already cover all your concerns.

@BladeMF
Copy link
Author

BladeMF commented Mar 29, 2024

@staabm I can try and outline my problems chronologically:

  1. When a test fails when comparing two large objects (e.g. Doctirne entities + comparing if they are the same object) and the test fails, then the printout of the error is 1.3G and is practically unreadable. You can't even know what test failed because it is lost in the endless printing. That is solved either by:
    1.1. Making a smaller output. The drawback being that what if I need 4 levels deep, but not all properties (e.g. ignore Doctrine's entity manager, otherwise I want all properties and all levels because I am comparing the whole data structure and what if the difference is on level 4)? This solution though is a very good default setting.
    1.2. Making a configurable exporter. That way Doctrine can provide their own meaningful exporter. Currently they have the same provider for the Symfony's dump function.
  2. Providing very large objects from a data provider. Here I am inclined to agree that there should be a better test design, alghough I have the suspicion that there exist cases where that cannot be avoided.
    2.1. Exporting the text representation in this case should not really be more than a level or two. Even better, do not export at all, but rely on the entry's description.

@sebastianbergmann sebastianbergmann changed the title Make Exporter configurable/customisable Make export of objects customizable Mar 29, 2024
@sebastianbergmann
Copy link
Owner

@BladeMF Thank you for explaining your use case(s).

What do you think about sebastianbergmann/exporter#56? The idea is similar to #5773, but instead of completely swapping out SebastianBergmann\Exporter\Exporter this would be limited to customizing how SebastianBergmann\Exporter\Exporter handles objects. One or more custom implementations of SebastianBergmann\Exporter\ObjectExporter could be configured in PHPUnit's XML configuration file.

@BladeMF
Copy link
Author

BladeMF commented Mar 30, 2024

Yes, I really like that implementation. I left just one comment explaining what an example implementation of my exporter would look like.
I did not look at the shortened version though.

@BladeMF
Copy link
Author

BladeMF commented Mar 30, 2024

@sebastianbergmann BTW, thank you for your work. If not for Phpunit we who want to write proper code in PHP would not have any tools.

@sebastianbergmann sebastianbergmann added this to the PHPUnit 11.2 milestone Mar 31, 2024
@sebastianbergmann sebastianbergmann removed a link to a pull request Mar 31, 2024
8 tasks
@sebastianbergmann sebastianbergmann removed this from the PHPUnit 11.3 milestone Jul 26, 2024
@janedbal
Copy link
Contributor

How should we understand that removal from 11.3 milestone? Is this not planned anymore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-runner CLI test runner type/enhancement A new idea that should be implemented
Projects
None yet
Development

No branches or pull requests

5 participants