symfony/symfony-docsPublic

NotificationsYou must be signed in to change notification settings
Fork5.3k
Star2.3k

[Testing] Add docs for new WebTestCase assertions#11271

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

wouterj merged 1 commit intosymfony:masterfromPierstoval:assertions

Apr 7, 2019

Merged

[Testing] Add docs for new WebTestCase assertions#11271

wouterj merged 1 commit intosymfony:masterfromPierstoval:assertions

Apr 7, 2019

Conversation

Copy link

Contributor

Pierstoval commentedApr 1, 2019

Fixes#11266 , as ofsymfony/symfony#30813

carsonbot added the Status: Needs Review label

Apr 1, 2019

Pierstoval changed the title~~Add basics of new WebTestCase assertions~~[Testing] Add docs for new WebTestCase assertions

Apr 1, 2019

Copy link

ContributorAuthor

Pierstoval commentedApr 1, 2019

Should I add examples for every assertion? To me they all seem self-explanatory, but I could be more exhaustive with examples 😄

Copy link

ContributorAuthor

Pierstoval commentedApr 1, 2019

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 👍

Pierstoval mentioned this pull request

Apr 1, 2019

[4.3] New assertions for WebTestCase#11266

Closed

Copy link

Contributor

OskarStark commentedApr 2, 2019

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.
I think the user should what is done internally. An idea could be to write it that 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 manyversionadded directive (I mean for every new method one). I am for grouping them under a sub-headline. and add only oneversionadded.

What do you think @symfony/team-symfony-docs ?

OskarStark reviewed

Apr 2, 2019

View reviewed changes

Copy link

Contributor

OskarStark left a comment

Choose a reason for hiding this comment

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

I left some thoughts which could improve the docs a bit. 💭
You should not change everything before we heard some other thoughts from the docs-team.

friendly ping @symfony/team-symfony-docs ❤️

testing/functional_tests_assertions.rst OutdatedShow resolvedHide resolved

testing/functional_tests_assertions.rstShow resolvedHide resolved

testing/functional_tests_assertions.rst OutdatedShow resolvedHide resolved

fabpot requested changes

Apr 2, 2019

View reviewed changes

testing.rst OutdatedShow resolvedHide resolved

testing/functional_tests_assertions.rst OutdatedShow resolvedHide resolved

testing/functional_tests_assertions.rst Outdated

		..note::

		These assertions will not work with `symfony/panther`_ as they use the
		``Request`` and ``Response`` objects coming from the ``HttpFoundation``

Copy link

Member

Choose a reason for hiding this comment

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

I would remove the explanation about why they don't work with Panther (nobody cares). Wouldn't it be possible to make them work?

Copy link

Contributor

Choose a reason for hiding this comment

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

Maybe not adding it directly here, but adding a Troubleshooting headline at the end and explain the panther case sounds helpful to me.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

To make them work we should make all assertions compatible with bothHttpFoundation andBrowserKit classes (especiallyRequest andResponse), and make sure theBrowser class used is also compatible with Panther.

Copy link

Member

fabpotApr 2, 2019

Choose a reason for hiding this comment

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

... which is quite possible, right? I suppose you meant "constraints" and not "assertions" here.

Copy link

ContributorAuthor

PierstovalApr 2, 2019

Choose a reason for hiding this comment

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

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 😄

testing/functional_tests_assertions.rst OutdatedShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Apr 2, 2019

OskarStark added the PHPUnitBridge label

Apr 2, 2019

Copy link

ContributorAuthor

Pierstoval commentedApr 2, 2019

Thanks for the review! I'm a bit busy this week but I'll fix everything as soon as possible 👍

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Apr 2, 2019

Copy link

ContributorAuthor

Pierstoval commentedApr 2, 2019

I fixed many of your reviews, and I think we should also plan to make the assertions compatible with Panther for more flexibility 👍

OskarStark reviewed

Apr 2, 2019

View reviewed changes

testing/functional_tests_assertions.rst OutdatedShow resolvedHide resolved

testing.rst OutdatedShow resolvedHide resolved

OskarStark reviewed

Apr 3, 2019

View reviewed changes

Copy link

Contributor

OskarStark left a comment

Choose a reason for hiding this comment

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

I left some minor comments!

testing/functional_tests_assertions.rst OutdatedShow resolvedHide resolved

testing/functional_tests_assertions.rstShow resolvedHide resolved

testing.rst OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

Pierstoval commentedApr 3, 2019•
edited
Loading

@OskarStark Implemented your reviews 😉

OskarStark approved these changes

Apr 3, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 3, 2019

Copy link

ContributorAuthor

Pierstoval commentedApr 3, 2019

About the Panther compatibility, I think we should update Panther itself first, because ofthese lines throwing an exception ongetRequest() andgetResponse().

wouterj reviewed

Apr 6, 2019

View reviewed changes

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.

Copy link

Member

wouterjApr 6, 2019

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

PierstovalApr 7, 2019

Choose a reason for hiding this comment

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

I just added what you suggested, what do you think? 🙂

testing/functional_tests_assertions.rst

		Response
		~~~~~~~~

		- ``assertResponseIsSuccessful()``

Copy link

Member

wouterjApr 6, 2019

Choose a reason for hiding this comment

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

Maybe it is usefull to have the full signature here or transform them all to:method:`` links?

Copy link

Contributor

OskarStarkApr 6, 2019

Choose a reason for hiding this comment

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

We decided to not do this because Symfony API isn’t available anymore.

Yesterday I talked to@Nyholm,@dmaicher and@xabbuh about fixing this by unsung github. MaBe you can talk to each other directly

Copy link

Member

wouterjApr 6, 2019

Choose a reason for hiding this comment

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

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

Copy link

Contributor

OskarStarkApr 6, 2019

Choose a reason for hiding this comment

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

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

HeahDude added this to the4.3 milestone

Apr 6, 2019

Add docs of new WebTestCase assertions

3b1a1a1

wouterj added the ⭐️ EU-FOSSA Hackathonhttps://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming label

Apr 7, 2019

Copy link

Member

wouterj commentedApr 7, 2019•
edited
Loading

Great! Thanks for documenting this feature! (and by this merge, your PR has become part of the EUFOSSA hackathon as well 😃 )

wouterj merged commit3b1a1a1 intosymfony:master

Apr 7, 2019

wouterj added a commit that referenced this pull request

Apr 7, 2019

feature#11271[Testing] Add docs for new WebTestCase assertions (Pie…

9b550c6

…rstoval)This PR was merged into the master branch.Discussion----------[Testing] Add docs for new WebTestCase assertionsFixes#11266 , as ofsymfony/symfony#30813Commits-------3b1a1a1 Add docs of new WebTestCase assertions

Pierstoval deleted the assertions branch

April 7, 2019 14:02

Labels

⭐️ EU-FOSSA Hackathon

https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming

PHPUnitBridge Status: Reviewed

Movatterモバイル変換

Uh oh!

[Testing] Add docs for new WebTestCase assertions#11271

[Testing] Add docs for new WebTestCase assertions#11271

Uh oh!

Conversation

Pierstoval commentedApr 1, 2019

Uh oh!

Pierstoval commentedApr 1, 2019

Uh oh!

Pierstoval commentedApr 1, 2019

Uh oh!

OskarStark commentedApr 2, 2019

Uh oh!

OskarStark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Pierstoval commentedApr 2, 2019

Uh oh!

Pierstoval commentedApr 2, 2019

Uh oh!

Uh oh!

Uh oh!

OskarStark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pierstoval commentedApr 3, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Pierstoval commentedApr 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj commentedApr 7, 2019• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Pierstoval commentedApr 3, 2019•
edited
Loading

wouterj commentedApr 7, 2019•
edited
Loading