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.

Sign up for GitHub

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

Jump to bottom

Create new PHPUnit assertions for the WebTestCase #29990

Closed
wants to merge 1 commit into from
Closed

Create new PHPUnit assertions for the WebTestCase #29990

wants to merge 1 commit into from

Conversation

Pierstoval
Copy link
Contributor

@Pierstoval Pierstoval commented Jan 25, 2019 •
edited

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

I got inspirations from Laravel Dusk's assertions to bring new assertion capabilities to the WebTestCase in order to ease functional testing.

WDYT?

Edit: Here's the list of the added assertions, as a summary of this PR:

  • assertResponseIsSuccessful
  • assertHttpCodeEquals
  • assertResponseHasHeader
  • assertResponseNotHasHeader
  • assertResponseHeaderEquals
  • assertResponseHeaderNotEquals
  • assertResponseRedirects
  • assertPageTitleEquals
  • assertPageTitleContains
  • assertClientHasCookie
  • assertClientNotHasCookie
  • assertClientCookieValueEquals
  • assertClientRawCookieValueEquals
  • assertResponseHasCookie
  • assertResponseNotHasCookie
  • assertResponseCookieValueEquals
  • assertResponseCookieValueNotEquals
  • assertSelectorExists
  • assertSelectorNotExists
  • assertSelectorContainsText
  • assertSelectorNotContainsText
  • assertInputValueEquals
  • assertInputValueNotEquals
  • assertRouteEquals
  • assertRequestAttributeValueEquals

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 27, 2019
dunglas
dunglas approved these changes Feb 12, 2019
View reviewed changes
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Cool!

@XuruDragon
Copy link

Thank you ! so tired of always recoding the same bits of tests... This will greatly increase testing :)

@Pierstoval
Copy link
Contributor Author

Pierstoval commented Feb 18, 2019 •
edited

Thanks!

Friendly update to the reviewers: I added a slight summary of the assertions in the PR description so we can directly see what's added, for easier review.

Waiting for more reviews 👍

Edit: And it's rebased & squashed 🌮

localheinz
localheinz suggested changes Feb 20, 2019
View reviewed changes
Copy link
Contributor

@localheinz localheinz left a comment

Choose a reason for hiding this comment

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

Why are there no tests?

@Pierstoval
Copy link
Contributor Author

@localheinz:

Why are there no tests?

I guess because the KernelTestCase is not tested?

@Pierstoval
Copy link
Contributor Author

Pierstoval commented Mar 17, 2019 •
edited

Friendly ping to @dunglas and @stof and @nicolas-grekas

I fixed the compatibility with PHPUnit8 by using the same technique as https://github.com/symfony/symfony/pull/30474/files

However, the aforementioned PR introduces an issue IMO: the doSetUp() and doTearDown() don't include the : void return type in the "PHPUnit <8 compatible" version of the class. Since Symfony 4 is compatible with PHP 7.1 only, I added the return type in all versions of the TestCaseSetUpTearDownTrait to be sure to not trigger the same issues than the one we have with PHPUnit8's BC break.

@Pierstoval
Copy link
Contributor Author

The PR is ready, still waiting for reviews 😀

fabpot
fabpot reviewed Mar 31, 2019
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Looks interesting. I have not reviewed all assertions but I've made some cosmetic recommendations.

src/Symfony/Bundle/FrameworkBundle/Test/WebTestAssertions.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/FrameworkBundle/Test/WebTestAssertions.php Outdated Show resolved Hide resolved
@Pierstoval
Copy link
Contributor Author

Thanks @fabpot for the review, I fixed the cosmetic things.

fabbot.io's error seems like a false positive to me, since the phpdoc is necessary to sort of "warn" about the fact that the return type is void with PHPUnit 8.

I squashed everything, so the PR seems finished to me, unless anyone has another comment or idea I could work on 😄

fabpot
fabpot reviewed Mar 31, 2019
View reviewed changes
src/Symfony/Component/Form/Test/TestCaseSetUpTearDownTrait.php
* @return void
*/
private function doSetUp()
private function doSetUp(): void
Copy link
Member

Choose a reason for hiding this comment

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

Why this change and the one on the Validator component? They don't seem related to your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I squashed the commit with all the others, but this is intended to fix the fact that PHPUnit 8 needs the type-hint, and not PHPUnit 7.

As Symfony needs PHP 7.1, the void return type can be added with no harm, and since these traits are new as of the upcoming v4.3, IMO they can benefit from this return type right from the start.

I may separate the fix into another commit or into another PR if needed, but since I also need to add such trait in the new WebTestAssertions, I thought it would be also necessary to bring the fix to other traits at the same place.

WYDT? Keep it as-is? New commit/PR?

Copy link
Member

Choose a reason for hiding this comment

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

For 4.2 and should be applied to all similar traits if there are more similar cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I split the change and put it in another PR targeted on 4.2?

Copy link
Member

Choose a reason for hiding this comment

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

So, let's remove this change from here and let's create another PR for 4.2.

@fabpot
Copy link
Member

fabpot commented Mar 31, 2019

@Pierstoval I think we also need some tests for these new assertions.

@Pierstoval
Copy link
Contributor Author

@fabpot I'd be interested to know how I could test this, maybe I need a bit of coaching on this. I can create a single test and you could tell me if the direction is good or not 🙂

@fabpot fabpot mentioned this pull request Apr 1, 2019
Merged
@fabpot
Copy link
Member

fabpot commented Apr 1, 2019

@Pierstoval I've taken over this PR in #30813 where I've refactored the code and added tests.

@fabpot
Copy link
Member

fabpot commented Apr 1, 2019

closing in favor of #30813

@fabpot fabpot closed this Apr 1, 2019
fabpot added a commit that referenced this pull request Apr 1, 2019
@fabpot
feature #30813 New PHPUnit assertions for the WebTestCase (Pierstoval…
17a3ccf 
…, 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
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot requested changes

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@renanbr renanbr renanbr left review comments

@mhujer mhujer mhujer left review comments

@stof stof stof left review comments

@sstok sstok sstok left review comments

@ro0NL ro0NL ro0NL left review comments

@XuruDragon XuruDragon XuruDragon left review comments

@localheinz localheinz localheinz requested changes

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
Feature Status: Needs Work
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

None yet