Provide test functionality in traits #218
Conversation
| /** | ||
| * Provides a HTTP client for getting responses from your application | ||
| */ | ||
| trait AppClient |
There was a problem hiding this comment.
Is this a good name? Perhaps just HttpClient or AppHttpClient or HttpAdapter? I think the name should show that this is not a client for your proxy, but a client to simulate requests to your application.
There was a problem hiding this comment.
i wonder if it makes sense that all of this is a trait. could we have something like a client provider instead?
| * | ||
| * @var HttpClient | ||
| */ | ||
| protected $client; |
There was a problem hiding this comment.
can this be private? what does it mean exactly when a trait has a private property, is it hidden from the class that uses the trait? and what if a class already has a non-private $client? then the user would need to remap the property?
from @stof in https://github.com/FriendsOfSymfony/FOSHttpCache/pull/224/files#r33294271
|
@ddeboer can you wrap this one up? make things private where possible and update the testing doc? otherwise looks good to me. |
ddeboer
force-pushed
the
test-traits
branch
2 times, most recently
from
d32188f to
f3c59e6
Compare
|
Docs updated. |
| ~~~~~~~~~~~~~~~~ | ||
|
|
||
| Provides your tests with a ``getResponse`` method, which retrieves a URI from | ||
| your application through the HTTP cache:: |
There was a problem hiding this comment.
maybe "through a real HTTP call to go through the HTTP cache" to be more clear?
ddeboer
force-pushed
the
test-traits
branch
3 times, most recently
from
afc78e7 to
0ccd131
Compare
|
this does not fix #213, right? can we wrap this one up and then work on 213? |
| } | ||
|
|
||
| The ``UNDETERMINED`` state should never happen. If it does, it means that your | ||
| HttpCache is not correctly set into debug mode. |
There was a problem hiding this comment.
HttpCache? also, should we make this into another subscriber that one can simply activate in app_test.php?
There was a problem hiding this comment.
I’m not sure about this; we already had this in the docs and I just moved it. 👍 on a DebugSubscriber, but let’s do that in a separate PR.
| */ | ||
| abstract class VarnishTestCase extends ProxyTestCase | ||
| abstract class VarnishTestCase |
There was a problem hiding this comment.
this is broken. It needs to extend from the PHPUnit_Framework_TestCase
There was a problem hiding this comment.
it seems to not be needed, at least the travis tests did run through without. oh, or maybe phpunit now ignores classes called Test if they dont extend the phpunit TestCase class.
but anyways i added it back to avoid confusion.
There was a problem hiding this comment.
well, if it does not extend the base testcase class, it won't run as it won't be a PHPUnit testcase.
https://travis-ci.org/FriendsOfSymfony/FOSHttpCache/jobs/74553567 was running 78 tests while https://travis-ci.org/FriendsOfSymfony/FOSHttpCache/jobs/74556258 ran 112 tests
There was a problem hiding this comment.
Fix #202. separate http client from trait Make property and methods private Add traits to docs Reduce levels of inheritance
|
@ddeboer i rebased and did some cleanup only open point from my side is #218 (comment) - but i think we should just clean up the text to make it better understandable. providing a trait can happen in a separate PR. |
Provide test functionality in traits
Fix #202.
Advantage: users no longer have to extend our test classes, so they are free to extend their own (or their framework’s). Optionally, they can continue to extend
SymfonyTestCase/VarnishTestCase/NginxTest: these classes are there now purely for convenience, as they contain no logic but just collect the appropriate traits.Disadvantage: we have some code duplication in the getters, e.g.
getHostName(),getCachingProxyPort().Todo (if accepted):