[rfc][icedtea-web] ResourceTracker Download Refactor

Jiri Vanek jvanek at redhat.com
Fri Nov 7 13:38:01 UTC 2014


...snip...
>
>>
>> I'm not sure what to think about versionless method overloading:( How rest of
>> ITW is doing it?
>> If the (string,null) call is more wide spread, then I'm for it, and also to
>> replace other calls like
>> that.
>
> I have spent a lot of time looking at Resource:requestVersion and Resource:downloadVersion fields and it seems to me, that downloadVersion is useless, and requestVersion is used to support versioned JARs.
>
> I personally think we should change the functions to not require Version since they do not even use the field. Maybe I can post a patch so you can see what it looks like?

What I'm really afraid is, that the version will be forgoten. I was thinking about it, and it 
resulted (yaaaH! :D) that I would like to keep only (string, version) signature - and so fforce yu 
to use several more nulls:(

>
>>
>> Last nit you will not like - unit XOR reproducers for your new methods is
>> really wonted. I think
>> that reproducer - moreover being written as unittestm and only using prepared
>> resources, would be
>> completely enough.
>
> Technically, nearly every reproducer for IT-W tests this code.

fair point:)
>
> I am having a lot of trouble trying to create tests for this since there aren't any public methods changed. As well, I haven't been able to write any unit tests using local files because the Resource/Cache system prevents download+caching of local files from occurring.
>
> Given the number of reproducers we have, I feel like it is decently tested already.
>

I agree.
> Can you provide any suggestions or hints on what kind of tests to write?


What I had in mind were moreover unittests for new methods you have created.  To supply resources 
you can use our SeverAccess (in unittests) and so be done.

I actually do not wont you to add reproducere - you are more then right that there are tons of 
reproducers already testing it. But build some first wall deffence  in unittests.

This may be added as separate changeset.
>
>>
>> Also you did few:
>> - catch (Exception ex) {
>> +        } catch (IOException e) {
>> I would strongly discourage you from doing so...  Not only IO exception can
>> happen here.... And we
>> do not wont to die...
>
>
> Right. I have changed it back.
>
>>

Otherwise ok for head. after fixing the version.


If you insists on removing of version from this method, then I would encourage you to do so as 
separate changest.


J.




More information about the distro-pkg-dev mailing list