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

New PHPUnit assertions for the WebTestCase #30813

Merged
merged 2 commits into from Apr 1, 2019

Conversation

Projects
None yet
7 participants
@fabpot
Copy link
Member

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 fabpot force-pushed the fabpot:assertions branch from dcfe0d0 to e7a0c20 Apr 1, 2019

@fabpot

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

Green and ready!

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

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

This comment has been minimized.

Copy link
Member Author

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 fabpot:assertions branch 2 times, most recently from d6d8a72 to b7537e3 Apr 1, 2019

@fabpot fabpot force-pushed the fabpot:assertions branch from b7537e3 to 4f91020 Apr 1, 2019

@fabpot

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

Thank you @Pierstoval.

@fabpot fabpot merged commit 4f91020 into symfony:master Apr 1, 2019

0 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

fabpot added a commit that referenced this pull request Apr 1, 2019

feature #30813 New PHPUnit assertions for the WebTestCase (Pierstoval…
…, 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));

This comment has been minimized.

Copy link
@Pierstoval

Pierstoval Apr 1, 2019

Contributor

->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

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

@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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

@fabpot , done, see #30842

fabpot added a commit that referenced this pull request Apr 3, 2019

minor #30842 [FrameworkBundle] Rename WebTestAssertions -> WebTestAss…
…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

feature #11271 [Testing] Add docs for new WebTestCase assertions (Pie…
…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

This comment has been minimized.

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.