Skip to content

[VarDumper] Allow VarDumperTestTrait expectation to be non-scalar #25332

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

Merged

Conversation

romainneutron
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

At the moment, when using the VarDumperTestTrait in unit test, expecting data object is as follow:

class Toto
{
    private $foo;

    public function __construct($foo)
    {
        $this->foo = $foo;
    }
}

class MyTest extends \PHPUnit_Framework_TestCase
{
     use Symfony\Component\VarDumper\Test\VarDumperTestTrait;

    public function dummyTest()
    {
        $expected = <<<EOEXPECTED
Profiler\Tests\Model\CallGraph\Toto {
  -foo: "baz"
}
EOEXPECTED;

        $this->assertDumpEquals($expected, new Toto('baz'));
    }
}

The same test could be easily written like this with this change:

    public function dummyTest()
    {
        $this->assertDumpEquals(new Toto('baz'), new Toto('baz'));
    }

@romainneutron romainneutron force-pushed the var-dumper-non-scalar-expectation branch from 5b04647 to 6b5ab90 Compare December 5, 2017 12:52
@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Dec 5, 2017
@javiereguiluz
Copy link
Member

javiereguiluz commented Dec 6, 2017

I'm all in for simplifications, but don't you think this is now too confusing?

$this->assertDumpEquals(new Toto('baz'), new Toto('baz'));

The two arguments are the same, I'm testing a dump but no dump is provided, etc.

@nicolas-grekas
Copy link
Member

@javiereguiluz that's an real improvement over the current, where you have to maintain a dump for the expected side.
The fact that dumps are compared instead of the value is what makes the "dump" word legitimate.
The failure is much improved vs standard phpunit behavior, as the diff that will be generated then will be top notch.
I confirm my 👍 personally.

@Simperfit
Copy link
Contributor

So I guess we could use this feature to improve the test in symfony ?

@fabpot
Copy link
Member

fabpot commented Dec 11, 2017

I also like this for the better exception message you get instead of the standard PHPUnit one.

@fabpot
Copy link
Member

fabpot commented Dec 11, 2017

Thank you @romainneutron.

@fabpot fabpot merged commit 6b5ab90 into symfony:master Dec 11, 2017
fabpot added a commit that referenced this pull request Dec 11, 2017
… non-scalar (romainneutron)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[VarDumper] Allow VarDumperTestTrait expectation to be non-scalar

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

At the moment, when using the `VarDumperTestTrait` in unit test, expecting data object is as follow:

```php
class Toto
{
    private $foo;

    public function __construct($foo)
    {
        $this->foo = $foo;
    }
}

class MyTest extends \PHPUnit_Framework_TestCase
{
     use Symfony\Component\VarDumper\Test\VarDumperTestTrait;

    public function dummyTest()
    {
        $expected = <<<EOEXPECTED
Profiler\Tests\Model\CallGraph\Toto {
  -foo: "baz"
}
EOEXPECTED;

        $this->assertDumpEquals($expected, new Toto('baz'));
    }
}
```

The same test could be easily written like this with this change:

```php
    public function dummyTest()
    {
        $this->assertDumpEquals(new Toto('baz'), new Toto('baz'));
    }
```

Commits
-------

6b5ab90 [VarDumper] Allow VarDumperTestTrait expectation to be non-scalar
@romainneutron romainneutron deleted the var-dumper-non-scalar-expectation branch December 14, 2017 10:05
@fabpot fabpot mentioned this pull request May 7, 2018
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.

6 participants