[Testing] Add docs for new WebTestCase assertions #11271
Conversation
|
Should I add examples for every assertion? To me they all seem self-explanatory, but I could be more exhaustive with examples 😄 |
|
Also, to reference this page in a better way in the docs (so it can be easily accessible), I may need some advices and tips on where I could add links to this new page 👍 |
|
Hey Alex, thank you for this nice new feature and the documentation so far. To be honest I am not sure if replacing the explicit example with the shortcuts is the right way. $this->assertGreaterThan(
0,
$crawler->filter('html:contains("Hello World")')->count()
);
// or use a shortcut
$this->assertSelectorTextContains('html', 'Hello World');Not sure we should add so many What do you think @symfony/team-symfony-docs ? |
| .. note:: | ||
|
|
||
| These assertions will not work with `symfony/panther`_ as they use the | ||
| ``Request`` and ``Response`` objects coming from the ``HttpFoundation`` |
There was a problem hiding this comment.
I would remove the explanation about why they don't work with Panther (nobody cares). Wouldn't it be possible to make them work?
There was a problem hiding this comment.
Maybe not adding it directly here, but adding a Troubleshooting headline at the end and explain the panther case sounds helpful to me.
There was a problem hiding this comment.
To make them work we should make all assertions compatible with both HttpFoundation and BrowserKit classes (especially Request and Response), and make sure the Browser class used is also compatible with Panther.
There was a problem hiding this comment.
... which is quite possible, right? I suppose you meant "constraints" and not "assertions" here.
There was a problem hiding this comment.
Yep, I wasn't originally planning to use constraints, but since you updated the PR with it, we can make them compatible with both classes indeed 😄
|
Thanks for the review! I'm a bit busy this week but I'll fix everything as soon as possible 👍 |
|
I fixed many of your reviews, and I think we should also plan to make the assertions compatible with Panther for more flexibility 👍 |
|
@OskarStark Implemented your reviews 😉 |
|
About the Panther compatibility, I think we should update Panther itself first, because of these lines throwing an exception on |
testing.rst
Outdated
|
|
||
| This assertion will internally call ``$crawler->filter('html')``, which allows | ||
| you to use CSS selectors to filter any HTML element in the page and check for | ||
| its existence, attributes, text, etc. |
There was a problem hiding this comment.
As the new method is much nicer than the previous one, let's update the code example directly using this assertion. We can then reduce this .. versionadded:: directive to "The WebTestCase assertions were introduced in Symfony 4.3." and add the .. seealso:: directive below it.
There was a problem hiding this comment.
I just added what you suggested, what do you think? 🙂
| Response | ||
| ~~~~~~~~ | ||
|
|
||
| - ``assertResponseIsSuccessful()`` |
There was a problem hiding this comment.
Maybe it is usefull to have the full signature here or transform them all to :method:`` links?
There was a problem hiding this comment.
We cannot link to a specific method when using GitHub, so that's a bit sad. For class, it will be a good solution though. We must discuss whether it's usefull to link functions, or just like to the class on github directly
There was a problem hiding this comment.
in a PR github seems to have this method information. Otherwiese we could have a proxy wich calculates the needed line number on demand for the github url
|
Great! Thanks for documenting this feature! (and by this merge, your PR has become part of the EUFOSSA hackathon as well 😃 ) |
…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
Fixes #11266 , as of symfony/symfony#30813