[rfc][icedtea-web] fixing CacheReproducerTest and improving VersionedJarTest

[Jiri Vanek]

On 03/04/2015 04:55 PM, Jie Kang wrote:
>
>
> ----- Original Message -----
>> On 03/04/2015 04:37 PM, Jie Kang wrote:
>>> Hello,
>>>
>>>
>>> Also, just noticed:
>>>
>>> CacheLRUWrapper.java:
>>>
>>>       private boolean checkData () {
>>>       [...]
>>>                   if (!path.contains(cacheDir.getAbsolutePath())) {
>>>
>>> Here you use cacheDir.getAbsolutePath() but:
>>>
>>>       private String getIdForCacheFolder(String folder) {
>>>           int len = getCacheDir().getAbsolutePath().length();
>>>
>>> Here you use getCacheDir().
>>>
>>> I think you wanted to use getCacheDir() everywhere.
>>
>> Sure. will be fixed before push.
>>>
>>>
>>>
>>>
>>>
>>> And one follow-up question:
>>>
>>> CacheUtil gets cacheDir from CacheLRUWrapper with default constructor which
>>> has:
>>>
>>>       public CacheLRUWrapper() {
>>>        this(PathsAndFiles.RECENTLY_USED_FILE.getFile());
>>>       }
>>
>> Ugh, where have you found it? This constructor do not exists....
>
> Sorry, I meant:
>
>       public CacheLRUWrapper() {
>           this(PathsAndFiles.RECENTLY_USED_FILE.getFile(), PathsAndFiles.CACHE_DIR.getFile());
>       }
Ok. I guees this is settlet thenn.

> The stuff below is still an issue though.
>
>>>
>>>
>>> Does this mean that DeploymentConfiguration.KEY_USER_CACHE_DIR is not used
>>> anymore?
>>>
>>> This means that code like:
>>>
>>>    JNLPRuntime.getConfiguration().setProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR,
>>>    System.getProperty("java.io.tmpdir") + File.separator + "tempcache");
>>>
>>> won't work right?
>>
>> Yes. PAthsAndFiles arebased on that property,so runtime change will not take
>> effect. This may be subject of another changeset....
>>>
>>> Also, can you double-check if this works with changing the cache directory
>>> in itweb-settings? From what I remember, the code there sets the
>>> DeploymentConfiguration.KEY_USER_CACHE_DIR, but you may have changed it
>>> already.
>>>
>>>
>>> So for CacheUtil unit tests, how do we create a temporary cache now? The
>>> tests need to make sure CacheUtil functions actually work, but they should
>>> definitely not use the user's cache.
>>>
>>>
>>
>> Seems like you are right. That although the value from defaults is going
>> correctly, when changed by user then it is not. As this is old bug, just
>> made more visible by my chnages,  may I fix it as another changeset?
>
> Well please fix this as soon as possible.
>

sure. Probably today.

> Please make sure to get both use-cases below:
>
> 1) user changes cachedir to custom location using itweb-settings : this is broken with your patch (bug in PathsAndFiles?)

Sure!

>
> 2) for unit tests : we need to be able to set cache to a temporary location : CacheUtil unit tests need to test caching functions (clearCache, etc.), but not in user's cache.

I thought you were working on this? I guess I made this a bit more complicated now...

I'm not sure if this feature is desirable, however Most easy solution will be to create false CacheLruWrapper, or not? (Now I guess You know why it s probably better to not copy its value into static field...)
J.