[icedtea-web] RFC: Unit tests for BrowserAwareProxySelector

[Andrew Azores]

On 09/27/2013 07:32 PM, Omair Majid wrote:
> On 09/24/2013 02:46 PM, Omair Majid wrote:
>> Hi,
>>
>> The attached patch adds unit tests for BrowserAwareProxySelector and
>> makes minor changes to make testing it easier.
>>
>> It also removes duplicate logic of selecting a host and port depending
>> on the protocol into JNLPProxySelector.getFromArguments. The semantics
>> of 'sameProxy' are different between BrowserAwareProxySelector and
>> JNLPProxy selector; the socks proxy is included in firefox [1].
>>
> Ping. Can anyone review this?
>
> Thanks,
> Omair
>

Different filename in the license header of 
BrowserAwareProxySelectorTest.java.

Patch looks good to me, though the unit tests could be refactored a bit. 
I don't insist at all because it's at least very readable as it is now, 
but for example "new Proxy(Type.HTTP, new InetSocketAddress(PROXY_HOST, 
PROXY_PORT))" gets copy-pasted a lot and might've been worth keeping 
around as a static final Proxy PROXY_HTTP, and similar for the repeated 
Proxy with Type.SOCKS. Other helper methods could be made for the tests 
since they're all doing more or less the same work, eg these lines:

+        BrowserAwareProxySelector selector = new 
TestBrowserAwareProxySelector(config, browserPrefs);
+        selector.initialize();
+
+        List<Proxy> result = selector.getFromBrowser(new 
URI("http://example.org"));
+
+        assertEquals(1, result.size());
+        assertEquals(new Proxy(Type.HTTP, new 
InetSocketAddress(PROXY_HOST, PROXY_PORT)), result.get(0));

could probably be pulled out into a new helper method like 
testBrowserProxyResult(config, browserPrefs, PROXY_HTTP). This makes it 
a little messy perhaps because you need to also pass in PROXY_HOST and 
PROXY_PORT. But, since each time you're using them you've chosen the 
same two values, perhaps they could just be made fields for the whole class?

These lines too:

+        DeploymentConfiguration config = new DeploymentConfiguration();
+        config.setProperty(DeploymentConfiguration.KEY_PROXY_TYPE, 
String.valueOf(JNLPProxySelector.PROXY_TYPE_BROWSER));
+        Map<String, String> browserPrefs = new HashMap<String, String>();

Are common to all the new test methods. It just seems to me that the 
only differentiation between most of these tests is the values you put 
into the browserPrefs map and the rest is all common and could be 
factored out.

Anyway, like I said I don't insist on this refactoring, because the 
tests are still readable and make clear sense. The class is just a lot 
longer in LoC than it probably has to be :) your call on bothering with 
refactor or not. Just fix the filename in that license header before 
push at least though.

Thanks,

-- 
Andrew A