[rfc][icedtea-web] Resource/ResourceTracker clean up

[Andrew Azores]

On 05/09/2014 01:31 PM, Omair Majid wrote:
> * Andrew Azores <aazores at redhat.com> [2014-05-07 12:02]:
>>> ugh... I would like to stop this being pushed until the unitttest for
>>> *original* behavior are done. And then they are adapted to new "behavior".
>>> This part of itw is lacking the tests,  so here is actually no trace if
>>> behvaiour was preserved.
>>>
>>> Otherwise the patch is ok.
>> Alright. I've attached tests for Resource#status and for
>> ResourceTracker.selectByFlag in their current state. They can be fairly
>> easily reworked for the EnumSet patch once you verify that the tests are
>> sufficiently guarding against behaviour changes (or find that they aren't
>> and I fix it so they are!). These new tests should apply to HEAD and all
>> pass currently.
> Looks okay to me. Some of utility functions in the test class are
> unused. I assume that's intentional; please remove them if it's a
> mistake.

Oops, yes a mistake. They were used previously but I forgot to remove 
them after they weren't needed anymore.

>
>> +++ b/tests/netx/unit/net/sourceforge/jnlp/cache/ResourceTest.java
>> @@ -0,0 +1,155 @@
>> +    private static final Version VERSION1 = new Version("1.0");
> This is used in one one method, and the actual version is insignificant.
> Maybe call it SOME_VERSION or A_VERSION and move it to the method
> itself?

Sure.

>
>> +    @Test
>> +    public void testNewResourceIsUninitialized() throws Exception {
>> +        Resource res = getDummyResource("NewResource");
>> +        assertTrue("Resource should not have had any status flags set", hasFlag(res, UNINITIALIZED));
> This test reads a little funny. Normally names like 'dummy' (and 'stub',
> 'fake', 'temp') are for an object which is mostly incidental to the
> test. So a function named `getDummyResource` that returns the very thing
> this test is looking at seems a little odd. Maybe call it
> `createResource` or something?

True enough. The tests that have been written are all targeting the 
status field of the resource in particular and so the resource itself is 
"mostly incidental", but I guess when the test class is called 
ResourceTest...

>
>> +    private static Resource getDummyResource(String testName) throws MalformedURLException {
>> +        URL dummyUrl = new URL("http://example.com/applet" + testName + ".jar");
>> +        return Resource.getResource(dummyUrl, VERSION1, UpdatePolicy.ALWAYS);
> This doesn't create a global, right? Calling this method with the same
> argument multiple times returns new objects, right?
>
> Thanks,
> Omair
>

Unfortunately... no :( the Resource constructor is private and the 
Resource class keeps a static list of resources, so getResource() 
returns an existing matching resource if it can. This is actually based 
only on the resource location, hence why each test is requesting a 
resource with a different location. It seems to me like this behaviour 
should probably be in the ResourceTracker rather than Resource itself, 
but it is what it is. Maybe that part can also be revisited later.

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: resource-enumset-tests-2.patch
Type: text/x-patch
Size: 10036 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140509/09021f96/resource-enumset-tests-2.patch>