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:*-test] Use the new WebTestAssertionsTrait methods in the generated functional tests #381

Conversation

adrienlucas
Copy link
Contributor

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 : As pointed out in this article, we should not be calling the assertions using $this->assert* but instead with static::assert*.

Second commit : To go further, we can use the new assertion API provided by the WebTestCase.

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

@weaverryan
Copy link
Member

Hey @adrienlucas!

Thank you for this! So, let's look at the 2 commits independently:

  1. self:: vs $this-> for the assert functions. For this, we should be consistent with Symfony's core and what's recommended in Symfony's docs, which is $this-> currently. We could discuss that on an issue on symfony/symfony-docs. I don't feel too strongly, but MakerBundle will follow that lead.

  2. About using the new assertion API in WebTestCase. I <3 this idea. However, as this feature is only available in Symfony 4.3 or higher, we need to make the generated code smart: it needs to use the old code if the user doesn't have the new WebTestAssertions trait and new code if they do. We could determine that in the maker and pass it in as a variable. Could you make that change?

Thanks!

@adrienlucas
Copy link
Contributor Author

adrienlucas commented Apr 23, 2019

@weaverryan

  1. I get it, while being a little disappointed about it... Is there an explanation for using a non-statc call on a static method here ? I will fix it anyway, to keep consistency.
  2. Will do !

To go further : should i try to assess Panther installation too, and make the generated test extends the PantherTestCase when possible ?

@B-Galati
Copy link

For the 1st one, see sebastianbergmann/phpunit#1914

@adrienlucas adrienlucas force-pushed the fix-assertions-calls-in-tests-skeletons branch from cfb0715 to 5982f3c Compare May 6, 2019 09:57
@adrienlucas
Copy link
Contributor Author

@B-Galati thank you !

@weaverryan does the new version of this PR suits more the needs ?

should we try to use travis to test against the symfony/framework-bundle:dev-master dependency, or something else to ensure the two versions of the possibly generated functional tests are working ?

(i renamed the PR to reflect the new intent)

@adrienlucas adrienlucas changed the title [make:*-test] Fix assertions calls in tests skeletons [make:*-test] Use the new WebTestAssertionsTrait methods in the generated functional tests May 6, 2019
@weaverryan
Copy link
Member

Thank you Adrien!

@weaverryan weaverryan merged commit 5982f3c into symfony:master Jun 1, 2019
weaverryan added a commit 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
weaverryan added a commit that referenced this pull request Jun 14, 2019
This PR was merged into the 1.0-dev branch.

Discussion
----------

Fixing bad check for trait

Fixes one little detail from #381

Commits
-------

472012f Fixing bad check for trait
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants