Skip to content

New PHPUnit assertions for the WebTestCase #30813

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
merged 2 commits into from
Apr 1, 2019
Merged

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Apr 1, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets replaces #29990
License MIT
Doc PR n/a

While reviewing #29990, and working on some tests, I realized that we could do better by adding PHPUnit constraint classes in various components that are then used in WebTextCase.

Before

<?php

namespace App\Tests;

use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;

class DefaultControllerTest extends WebTestCase
{
    public function testSomething()
    {
        $client = static::createClient();
        $crawler = $client->request('GET', '/test');

        $this->assertSame(200, $client->getResponse()->getStatusCode());
        $this->assertContains('Hello World', $crawler->filter('h1')->text());
    }
}

After

<?php

namespace App\Tests;

use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;

class DefaultControllerTest extends WebTestCase
{
    public function testSomething()
    {
        $client = static::createClient();
        $client->request('GET', '/test');

        $this->assertResponseIsSuccessful();
        $this->assertSelectorTextContains('h1', 'Hello World');
    }
}

@fabpot
Copy link
Member Author

fabpot commented Apr 1, 2019

Green and ready!

@javiereguiluz
Copy link
Member

Minor comment: checking PHPUnit's assertions (https://phpunit.de/manual/6.5/en/appendixes.assertions.html) I saw that they don't define "negative asserts" (NotHas..., NotContains..., etc.) but here we're defining asserts in symmetric pairs: (assertResponseHasCookie() + assertResponseNotHasCookie(), assertInputValueSame() + assertInputValueNotSame(), etc.) Just in case you want to double check if we really want this. Thanks.

@fabpot
Copy link
Member Author

fabpot commented Apr 1, 2019

@javiereguiluz If you have a closer look, you will see that PHPUnit defines a Not alternative for almost all their assertions.

@fabpot fabpot force-pushed the assertions branch 2 times, most recently from d6d8a72 to b7537e3 Compare April 1, 2019 16:33
@fabpot
Copy link
Member Author

fabpot commented Apr 1, 2019

Thank you @Pierstoval.

@fabpot fabpot merged commit 4f91020 into symfony:master Apr 1, 2019
fabpot added a commit that referenced this pull request Apr 1, 2019
…, fabpot)

This PR was merged into the 4.3-dev branch.

Discussion
----------

New PHPUnit assertions for the WebTestCase

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | replaces #29990
| License       | MIT
| Doc PR        | n/a

While reviewing #29990, and working on some tests, I realized that we could do better by adding PHPUnit constraint classes in various components that are then used in WebTextCase.

**Before**

```php
<?php

namespace App\Tests;

use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;

class DefaultControllerTest extends WebTestCase
{
    public function testSomething()
    {
        $client = static::createClient();
        $crawler = $client->request('GET', '/test');

        $this->assertSame(200, $client->getResponse()->getStatusCode());
        $this->assertContains('Hello World', $crawler->filter('h1')->text());
    }
}
```

**After**

```php
<?php

namespace App\Tests;

use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;

class DefaultControllerTest extends WebTestCase
{
    public function testSomething()
    {
        $client = static::createClient();
        $client->request('GET', '/test');

        $this->assertResponseIsSuccessful();
        $this->assertSelectorTextContains('h1', 'Hello World');
    }
}
```

Commits
-------

4f91020 added PHPUnit assertions in various components
2f8040e Create new PHPUnit assertions for the WebTestCase
private function getResponseTester(Response $response): WebTestCase
{
$client = $this->createMock(KernelBrowser::class);
$client->expects($this->any())->method('getResponse')->will($this->returnValue($response));
Copy link
Contributor

Choose a reason for hiding this comment

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

->expects($this->any()) is useless because it doesn't do anything, I'm not sure we should keep encouraging this method when using mocks 🤔

@vudaltsov
Copy link
Contributor

@fabpot , why it is not suffixed with Trait according to Symfony coding standards?
https://symfony.com/doc/current/contributing/code/standards.html#naming-conventions

@fabpot
Copy link
Member Author

fabpot commented Apr 3, 2019

@vudaltsov Good catch! That's because I got the code from another PR and didn't think about that. Can you submit a PR to rename it properly?

@vudaltsov
Copy link
Contributor

@fabpot , done, see #30842

fabpot added a commit that referenced this pull request Apr 3, 2019
…ertionsTrait (vudaltsov)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[FrameworkBundle] Rename WebTestAssertions -> WebTestAssertionsTrait

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30813 (comment)
| License       | MIT
| Doc PR        |

Renamed according to the Symfony coding standards.

Commits
-------

2ae30a7 Rename WebTestAssertions -> WebTestAssertionsTrait
wouterj added a commit to symfony/symfony-docs that referenced this pull request Apr 7, 2019
…rstoval)

This PR was merged into the master branch.

Discussion
----------

[Testing] Add docs for new WebTestCase assertions

Fixes #11266 , as of symfony/symfony#30813

Commits
-------

3b1a1a1 Add docs of new WebTestCase assertions
@TomasVotruba
Copy link
Contributor

TomasVotruba commented Apr 13, 2019

Just wondering, the Symfony blog post mentions many assertsions that are not there.

E.g. assertClientCookieValueEquals()

When you search for this string in whole Symfony code (here), you see it was in previous PR, but never merged

Blog post should be updated with this PR, not previous one. It's very confusing now

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot deleted the assertions branch May 8, 2019 07:56
@fabpot fabpot mentioned this pull request May 9, 2019
weaverryan added a commit to symfony/maker-bundle that referenced this pull request Jun 1, 2019
… in the generated functional tests (adrienlucas)

This PR was merged into the 1.0-dev branch.

Discussion
----------

[make:*-test] Use the new WebTestAssertionsTrait methods in the generated functional tests

Hi, this PR is to fix the way of calling the assertions static methods of PHPunit in the generated unit and functional tests.

[First commit](8f4af14) : As pointed out in [this article](https://arkadiuszkondas.com/the-right-way-to-call-assertion-in-phpunit/), we should not be calling the assertions using `$this->assert*` but instead with `static::assert*`.

[Second commit](cfb0715) : To go further, we can use [the new assertion API](symfony/symfony#30813) provided by the `WebTestCase`.

I don't kown if this could be considered a BC break... but i hope not.

Commits
-------

5982f3c Use the new api provided by the WebTestAssertionsTrait
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.

8 participants