Extract test functionality #103
Conversation
|
this fails on travis for nginx (maybe your local php does not treat warnings as errors) is it ok if we tag beta without this being in? i consider test refactorings to not be a BC break, at least until we document how to use things provided by the bundle for functional tests. |
| @@ -11,6 +11,8 @@ | |||
|
|
|||
| namespace FOS\HttpCache\Tests; | |||
|
|
|||
| use FOS\HttpCache\Tests\PHPUnit\IsCacheHitConstraint; | |||
| use FOS\HttpCache\Tests\PHPUnit\IsCacheMissConstraint; | |||
There was a problem hiding this comment.
i have seen the distinction between Test namespace used for base test cases and other things people are supposed to reuse, and Tests for the implementation specific tests stuff that you are not supposed to reuse. don't know how common that is, but if the intention is that people can reuse this, we could use the Test namespace and place things in src/ to make clear its intended to be reused.
|
Yeah, this is still failing because I’m working on Varnish first, then Nginx. |
| ); | ||
|
|
||
| if (0 !== $returnVar) { | ||
| self::markTestSkipped(new \Exception()); |
There was a problem hiding this comment.
This is left-over from development, so should be removed. It does raise the question what we want to happen if someone runs phpunit on the lib when Varnish and/or Nginx is not installed. Should we throw errors, or allow for Varnish-only development too (for instance), and skip the Nginx tests?
There was a problem hiding this comment.
i think failing loudly is better than just skip in this case. developpers of the cache client lib should see what they do. they can still run only parts of the tests if they really want. or create a custom phpunit.xml file.
for people using the test cases in their own setup, it could be quite bad if launch errors are not properly reported.
There was a problem hiding this comment.
You’re right. I’m reverting this.
| } | ||
|
|
||
| /** | ||
| * Stop the proxy server |
Don't skip tests if Nginx is unavailable Refactor tests * Refactor tests so they use the new TestCases * Add PHPDocs * Update docs
Fix Nginx command-line syntax Fix Nginx port Rename getNginx() to getProxyClient() Fix tests on Nginx rm -rf * didn’t work correctly because of how ProcessBuilder escapes arguments. Clean up Add FOS headers
| */ | ||
| protected $nginx; | ||
|
|
||
| const CACHE_EXPIRED = 'EXPIRED'; |
|
Squashed. This is almost done. @dbu One or two last questions, please have a look. |
| @@ -44,9 +45,39 @@ | |||
| /** | |||
| * @var Varnish | |||
| */ | |||
| protected $varnish; | |||
| protected $proxyClient; | |||
There was a problem hiding this comment.
do you repeat this from the base class?
There was a problem hiding this comment.
Actually, no, $proxyClient is not in the base class. I just have in separately in Varnish|NginxTestCase to typehint a bit more specifically than ProxyInterface.
|
@FriendsOfSymfony/foshttpcache Please merge if you agree with everything here. By the way, created the foshttpcache team for easy mentions, not such much for us as for other people creating issues and PRs. |
|
nice, thanks! we might revisit the documentation to explain more, at some point. |
Fix #101.