Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Improve functional test workflow example#9711

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

Closed
JeroenDeDauw wants to merge2 commits intosymfony:masterfromJeroenDeDauw:patch-1

Conversation

@JeroenDeDauw
Copy link
Contributor

The example used to be Arrange Act Assert Act Assert, rather thanhttp://wiki.c2.com/?ArrangeActAssert

The example used to be Arrange Act Assert Act Assert, rather thanhttp://wiki.c2.com/?ArrangeActAssert
@javiereguiluz
Copy link
Member

@JeroenDeDauw thanks for proposing this change to improve Symfony Docs. However, the link you shared (http://wiki.c2.com/?ArrangeActAssert) is for unit tests and this doc is for functional tests, so I'm not sure about the proposed change.

@JeroenDeDauw
Copy link
ContributorAuthor

Oh, I did not see this particular article is about unit tests specifically. The AAA pattern does not just apply to unit tests. The benefits are just the same for unit tests, integration tests and edge-to-edge tests like the ones this section is about.

@javiereguiluz
Copy link
Member

I'm afraid we're going to close this without merging. I know it's sad to do that ... but the mentioned workflow is the one used throughout the docs and it's very popular among Symfony developers. I hope you understand it. Thanks!

@JeroenDeDauw
Copy link
ContributorAuthor

If an anti-pattern is present through the documentation then it is not surprising it will be used by many developers. Not sure how the wide usage of an anti-pattern is a reason for recommending it in the documentation. I doing so is rather harmful, which is why I created this PR. I hope you reconsider.

@Ocramius
Copy link
Contributor

This should likely be re-considered, as the AAA approach is indeed something to strive for in test automation, while multiple actions lead to quite messy test scenarios (both to be read and to be debugged). This applies to any level of testing, as it keeps pre-conditions, mutations and post-conditions atomic.

@javiereguiluz
Copy link
Member

@JeroenDeDauw maybe I didn't fully understand your proposal. Let's talk about an specific example taken from the Symfony Demo app:https://github.com/symfony/demo/blob/master/tests/Controller/BlogControllerTest.php#L64

That test follows the same workflow explained in the Symfony Docs:

  • Make a request;
  • Test the response;
  • Click on a link or submit a form;
  • Test the response;

Now, please tell me how can we refactor that code to follow this other proposed workflow:

  • Make a request;
  • Test the response;

Thanks!

@JeroenDeDauw
Copy link
ContributorAuthor

I only see a single assert in that test. The name could definitely be better though. All it tells me is that the test is about new comments, without informing me what is actually tested, forcing me to look at implementation details. With such a general name it is also tempting to dump all tests related to new comments in a single test method, violating Arange-Act-Assert and making it even more difficult to figure out what the actual intent of the test is.

Instead of

TestAllTheThings(    Make a request;    Test the response;    Click on a link or submit a form;    Test the response;)

Do

TestWhenFooThenBar(    Make a request;    Assert Bar;)TestWhenBazThenBah(    Make a request;    Click on a link or submit a form;    Assert Bah;)

@javiereguiluz
Copy link
Member

@JeroenDeDauw thanks for the details! I understand you now. I thought you wanted to remove the"Click on a link or submit a form" step ... but that's not true. You want to remove the first"Test the response" to not make too many things in the same test. I fully agree, so I have tweaked a bit the original pull request. Thanks!

@JeroenDeDauw
Copy link
ContributorAuthor

Looks good! :)

javiereguiluz reacted with thumbs up emoji

@javiereguiluz
Copy link
Member

@JeroenDeDauw sorry for the misunderstanding. This is now merged ... so thank you and congrats on your first Symfony Docs contribution. Also, we've merged it on 2.7 (the oldest maintained branch) and we'll merge it up automatically to the rest of branches.

javiereguiluz added a commit that referenced this pull requestMay 24, 2018
…aviereguiluz)This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes#9711).Discussion----------Improve functional test workflow exampleThe example used to be Arrange Act Assert Act Assert, rather thanhttp://wiki.c2.com/?ArrangeActAssertCommits-------3c0a54a Reword36cb8a2 Improve functional test workflow example
@JeroenDeDauwJeroenDeDauw deleted the patch-1 branchMay 24, 2018 15:45
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

3 participants

@JeroenDeDauw@javiereguiluz@Ocramius

[8]ページ先頭

©2009-2025 Movatter.jp