Add ProxyTestCase #122

ddeboer · 2014-08-03T18:36:40Z

This depends on FriendsOfSymfony/FOSHttpCache#103.

It adds:

ProxyTestCase for functional cache tests in Symfony2 bundles
A @clearCache annotation that will clear the configured proxy before each test case starts. This is automatic in the lib’s AbstractProxyClientTestCase, but my experience with Symfony apps is that usually only one or two different test classes are extended, so doing stuff automatically can really be a pain then. It’s better to add some functionality behind a PHPUnit annotation that users can decide when they want the behaviour.
Some config for starting the proxy server in tests:

fos_http_cache:
    proxy_server:
        varnish:
            binary: /usr/sbin/varnishd
            port: 8080
            config_file: /etc/varnish/default.vcl

Some problems:

The ProxyTestCase is similar in some ways to the lib’s testcase. However, we cannot easily re-use that (except with traits) because we need to extend Symfony’s WebTestCase here for ProxyTestCase to have access to the container and be useful in a Symfony app.

ddeboer · 2014-08-03T18:37:35Z

Resources/config/varnish.xml

+            </call>
+            <call method="setPort">
+                <argument>%fos_http_cache.proxy_server.varnish.port%</argument>
+            </call>


Other setters may need to be added here, depending on the config that we allow.

dbu · 2014-08-04T06:24:22Z

cool idea! i guess we just have to live with a bit of duplication. this will need some documentation and we might add one or two integration tests using this test case to the bundle so that the test infrastructure is tested and people can find an example.

ddeboer · 2014-08-04T06:39:41Z

Resources/config/varnish.xml

+            <argument type="collection">
+                <argument key="%fos_http_cache.proxy_process.curlopt_forbid_reuse%">true</argument>
+            </argument>
+        </service>


Should we add the test client under proxy_server instead, or reorganise our service names in another way? Now we have proxy_client, proxy_server, and proxy.test_client. I imagine this will get confusing for people. 😉

And perhaps this test client could be combined with the custom guzzle_client for the proxy client, after all: they are constructed the same way. What if we always construct a guzzle_client, custom or not, and use that as the test client, too?

not sure if we need the same client for tests as for the application. could be wrong for some projects.

for the naming, i would prefer to say fos_http_cache.test.proxy_server.varnish. its not meant to control the "real" varnish through php, and we should not even suggest that this could be a good idea. then it would be test.proxy_server and test.proxy_server.client.

Let’s keep the clients separate, then.

I now have fos_http_cache.proxy.test_client.varnish, but I like your suggestion of prefixing with test instead. This yields the following config.yml, which makes it really clear that this is for testing purposes only:

fos_http_cache: test: proxy_server: varnish: ~

dbu · 2014-08-04T13:55:31Z

Resources/config/varnish.xml

@@ -6,6 +6,9 @@

    <parameters>
        <parameter key="fos_http_cache.proxy_client.varnish.class">FOS\HttpCache\ProxyClient\Varnish</parameter>
+        <parameter key="fos_http_cache.proxy_client.varnish.test_client.class">Guzzle\Http\Client</parameter>
+        <parameter key="fos_http_cache.proxy_server.varnish.class">FOS\HttpCache\Test\Proxy\VarnishProxy</parameter>
+        <parameter key="fos_http_cache.proxy_process.curlopt_forbid_reuse" type="constant">CURLOPT_FORBID_REUSE</parameter>


why not just inline that? (this is a real question, not rhetorical ;-)

Yeah, that doesn't seem to work with PHP constants. Or do you know a way?

ah, seems so. okay, learned something :-)

dbu · 2014-08-06T06:13:43Z

i restarted the build now that the PR on the component is merged.

once this is merged, its time for RC1 i would say.

ddeboer · 2014-08-06T06:29:07Z

Yep, RC here we come. I hope to have this ready later today.

ddeboer · 2014-08-06T14:32:24Z

@dbu Have a look, especially at the config structure and docs. If you agree, I’ll write some config tests so we can merge this.

dbu · 2014-08-06T15:25:51Z

DependencyInjection/Configuration.php

+                            ->defaultValue('X-Cache')
+                            ->info('HTTP cache hit/miss header')
+                        ->end()
+                        ->arrayNode('proxy_server')


->info('Configure how the caching proxy instance should be started for your tests')

dbu · 2014-08-06T15:34:18Z

cool, i think this is good. added some notes, and the tests are failing - apart from that lets squash and merge.

dbu · 2014-08-06T15:35:26Z

Resources/doc/reference/configuration/test.rst

+
+**type**: ``string`` **required**
+
+Path to a VCL file. For example Varnish configurations, see


can we only have one for the whole project? or can we also test with specific vcl files?

Only one. Do you think having more would make sense? I think we only need to offer testing against a production-like Varnish setup, so only one VCL (with possible includes). I know that we test against multiple VCLs in the library (especially user context), but there it makes sense: we’re testing the library’s compatibility with different kinds of VCLs. This kind of testing should not be done by users of the bundle, in my opinion.

indeed, good point. i agree.

ddeboer · 2014-08-07T12:12:13Z

Squashed. Have a look and merge when happy. Scrutinizer score dropped quite a bit, but I propose to merge this first and then go over all issues one more time (configuration near-duplication being the hardest, I guess). Would by nice to tag this RC soonish, as I’m using this in a project now. 😄

dbu · 2014-08-07T12:44:03Z

Test/ProxyTestCase.php

+     *
+     * @return \Guzzle\Http\Client
+     */
+    public function getClient()


getClient and getProxyClient are the same. i would vote for the explicit getProxyClient. and a protected method, its not supposed to be called from outside.

ddeboer · 2014-08-07T14:12:50Z

They return different services. Do you mean we should drop the test client and only use proxy client? I think we decided to keep the test client separate.

dbu · 2014-08-07T15:16:51Z

ups, i got confused, sorry. should we call this getTestClient and have a line in the phpdoc explaining what it is for?

ddeboer · 2014-08-07T15:45:38Z

Should we then rename getClient() in the library to getTestClient(), too? (To enforce consistency here is one of the reasons I prefer an interface.)

dbu · 2014-08-08T11:28:32Z

that would make sense. getClient is so generic, it does not mean much.

sorry, i am confused again: what does getClient return? a symfony web client to talk to the test varnish server? or a httpcache proxy client to talk to the test varnish server? if the later, what is the difference to the normal proxy client?

ddeboer · 2014-08-08T12:04:26Z

Heh this confusion perhaps calls for more extensive docs on this feature. :)

getClient()/getTestClient() returns a Guzzle HTTP client that has base_url (as configured under proxy_client) as it's endpoint. You can use this to test calls (mostly GET/HEAD) to the backend application through a live proxy.

getProxyClient() returns an instance of our Varnish or Nginx ProxyClient classes (which are wrapped by the CacheManager). These can be used to do manual purge, ban etc., though I think you shouldn't want to do this in application tests (of course, in our lib, we do).

stof · 2014-08-08T12:08:32Z

@ddeboer maybe it should be named getHttpClient then ?

ddeboer · 2014-08-08T12:15:24Z

@stof Makes sense: getHttpClient or getTestHttpClient distinguishes it more clearly from Symfony's built-in test client.

dbu · 2014-08-08T13:02:48Z

+1 for getHttpClient. we are in a test, so getTestHttpClient is kind of overloaded, but getHttpClient sounds descriptive. and say waht you wrote in your comment in the phpdoc for those unsure what it is.

stof · 2014-08-08T13:04:59Z

agreed with @dbu. Thus, this http client is not specific to tests. It is a real HTTP client (you can use it to load wikipedia if you want)

ddeboer · 2014-08-08T19:37:21Z

Okay, renamed to getHttpClient() and made the methods protected. Please have a last look and then I’ll squash and merge.

dbu · 2014-08-09T06:22:37Z

Resources/doc/reference/configuration/proxy-client.rst

-application. Used for invalidation with paths.
+The hostname (or base URL) where users access your web application. The base
+URL may contain a path. If you access your web application on a port other than
+ 80, include that port:


remove leading whitespace

dbu · 2014-08-09T06:25:53Z

cool. i only found cosmetics. feel free to squash and merge.

as you made the methods procteded: do you drop FriendsOfSymfony/FOSHttpCache#113? i would be ok to keep it in, in the end. the common doc and ensured consistency has convinced me its worth it ;-)

Add documentation Fix config and add docs Fix undefined index Fix config key name Fix test Fix fetching container

Rename to getHttpClient() * Rename getClient() to getHttpClient() * Make convenience methods protected * Remove getProxyClient() method from ProxyTestCase * Prefix base_url with "http://" so we can use it safely for the HTTP client * Update docs Improve type hints Remove leading whitespace

ddeboer · 2014-08-09T13:44:22Z

Fixed the cosmetics.

We can have the interface for the custom assertions, but if we want to include getResponse() and getHttpClient() we have to make them public (again)? So it’s a trade-off between consistency and reduced duplication and having a better API. Which do you prefer?

Add ProxyTestCase

dbu · 2014-08-11T07:52:49Z

great! ready for RC now?

ddeboer reviewed Aug 3, 2014
View reviewed changes

ddeboer reviewed Aug 4, 2014
View reviewed changes

ddeboer changed the title ~~Add ProxyTestCase~~ [WIP] Add ProxyTestCase Aug 4, 2014

dbu reviewed Aug 4, 2014
View reviewed changes

dbu mentioned this pull request Aug 6, 2014

Add more functional tests #120

Open

6 tasks

dbu reviewed Aug 6, 2014
View reviewed changes

dbu reviewed Aug 7, 2014
View reviewed changes

dbu reviewed Aug 9, 2014
View reviewed changes

ddeboer added 2 commits August 9, 2014 15:38

Add ProxyTestCase

cb142b3

Add documentation Fix config and add docs Fix undefined index Fix config key name Fix test Fix fetching container

ddeboer changed the title ~~[WIP] Add ProxyTestCase~~ Add ProxyTestCase Aug 10, 2014

ddeboer added a commit that referenced this pull request Aug 10, 2014

Merge pull request #122 from FriendsOfSymfony/testcase

9c6c5d4

Add ProxyTestCase

ddeboer merged commit 9c6c5d4 into master Aug 10, 2014

ddeboer deleted the testcase branch August 10, 2014 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ProxyTestCase #122

Add ProxyTestCase #122

ddeboer commented Aug 3, 2014

ddeboer Aug 3, 2014

dbu commented Aug 4, 2014

ddeboer Aug 4, 2014

dbu Aug 4, 2014

ddeboer Aug 4, 2014

dbu Aug 4, 2014

dbu Aug 4, 2014

ddeboer Aug 4, 2014

dbu Aug 4, 2014

dbu commented Aug 6, 2014

ddeboer commented Aug 6, 2014

ddeboer commented Aug 6, 2014

dbu Aug 6, 2014

ddeboer Aug 7, 2014

dbu commented Aug 6, 2014

dbu Aug 6, 2014

ddeboer Aug 6, 2014

dbu Aug 6, 2014

ddeboer commented Aug 7, 2014

dbu Aug 7, 2014

ddeboer commented Aug 7, 2014

dbu commented Aug 7, 2014

ddeboer commented Aug 7, 2014

dbu commented Aug 8, 2014

ddeboer commented Aug 8, 2014

stof commented Aug 8, 2014

ddeboer commented Aug 8, 2014

dbu commented Aug 8, 2014

stof commented Aug 8, 2014

ddeboer commented Aug 8, 2014

dbu Aug 9, 2014

dbu commented Aug 9, 2014

ddeboer commented Aug 9, 2014

dbu commented Aug 11, 2014


		type: ``string`` required

		Path to a VCL file. For example Varnish configurations, see

Add ProxyTestCase #122

Add ProxyTestCase #122

Conversation

ddeboer commented Aug 3, 2014

Choose a reason for hiding this comment

dbu commented Aug 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Aug 6, 2014

ddeboer commented Aug 6, 2014

ddeboer commented Aug 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbu commented Aug 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddeboer commented Aug 7, 2014

Choose a reason for hiding this comment

ddeboer commented Aug 7, 2014

dbu commented Aug 7, 2014

ddeboer commented Aug 7, 2014

dbu commented Aug 8, 2014

ddeboer commented Aug 8, 2014

stof commented Aug 8, 2014

ddeboer commented Aug 8, 2014

dbu commented Aug 8, 2014

stof commented Aug 8, 2014

ddeboer commented Aug 8, 2014

Choose a reason for hiding this comment

dbu commented Aug 9, 2014

ddeboer commented Aug 9, 2014

dbu commented Aug 11, 2014