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

Provide full test example #4712

Merged
merged 4 commits into from
Feb 24, 2015
Merged

Provide full test example #4712

merged 4 commits into from
Feb 24, 2015

Conversation

ifdattic
Copy link
Contributor

In my opinion providing the full code for the example only adds a few additional lines, but increases the value of the example greatly.

Of course the name for the test should probably be changed as couldn't think of a good one. ping @stof

Q A
Doc fix? yes
New docs? no
Applies to 2.3
Fixed tickets

In my opinion providing the full code for the example only adds a few additional lines, but increases the value of the example greatly.

Of course the name for the test should probably be changed as couldn't think of a good one. ping @stof

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3
| Fixed tickets |
{
$client = self::createClient();
$client->request('GET', $url);
<?php
Copy link
Member

Choose a reason for hiding this comment

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

you should remove the open tag, we never use it in the docs.

@stof
Copy link
Member

stof commented Dec 30, 2014

Why pinging me here ?

@ifdattic
Copy link
Contributor Author

Mostly because of https://twitter.com/_md/status/545597981270241280

As I don't really like the name of the test ApplicationAvailabilityFunctionalTest would be nice to get some feedback from you also.

$this->assertTrue($client->getResponse()->isSuccessful());
}

public function provideUrls()
Copy link
Contributor

Choose a reason for hiding this comment

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

i would like to rename this to UrlProvider, this would be consistent with the PHPUnit manual

Copy link
Member

Choose a reason for hiding this comment

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

I think this naming is fine (I always use the getPageIsSuccessfulData convention, but this one is good to understand for beginners too)

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 personally use ...Provider, makes it clear that this function is being used as a data provider for some test.

Copy link
Member

Choose a reason for hiding this comment

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

I think both getPageIsSuccessfulData() and urlProvider() are fine.

@timglabisch
Copy link
Contributor

👍

array('/archives'),
// ...
);
/** @dataProvider provideUrls */
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 actually like to have this as a "real" docblock:

/**
 * @dataProvider provideUrls
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, me too.
Will change it after the decision about the provider function name will be made.

{
$client = self::createClient();
$client->request('GET', $url);
// src/AppBundle/Tests/ApplicationAvailabilityFunctionalTest.php

Copy link
Member

Choose a reason for hiding this comment

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

you should remove this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the + // src/AppBundle/Tests/ApplicationAvailabilityFunctionalTest.php one ?

Copy link
Contributor

Choose a reason for hiding this comment

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

correct

Copy link
Member

Choose a reason for hiding this comment

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

Nope the empty line after the line you mentioned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@wouterj
Copy link
Member

wouterj commented Feb 22, 2015

One minor fix away from finished imo.

@weaverryan
Copy link
Member

I like it. Thanks Andrew!

@weaverryan weaverryan merged commit 9832e23 into symfony:2.3 Feb 24, 2015
weaverryan added a commit that referenced this pull request Feb 24, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

Provide full test example

In my opinion providing the full code for the example only adds a few additional lines, but increases the value of the example greatly.

Of course the name for the test should probably be changed as couldn't think of a good one. ping @stof

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3
| Fixed tickets |

Commits
-------

9832e23 Update tests.rst
4df1fe1 Update tests.rst
d4907ca Update tests.rst
d4ab971 Provide full test example
@ifdattic ifdattic deleted the patch-5 branch February 24, 2015 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants