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

[Jie Kang]

----- Original Message -----
> >>>>>>>>> 1) user changes cachedir to custom location using itweb-settings :
> >>>>>>>>> this
> >>>>>>>>> is broken with your patch (bug in PathsAndFiles?)
> >>>>>>>>
> >>>>>>>> Sure!
> >>>>>>>
> >>>>>>> Hmm. Looking to it now, it stopepd to gave sense to me.
> >>>>>>>
> >>>>>>> The cahe si already selectable via XDG variables. Why to make it more
> >>>>>>> complicated by completly custom value?
> >>>>>>>
> >>>>>>> Same is valid for logging.
> >>>>>>>
> >>>>>>> Is the chengable target of those two really de		sirable?
> >>>>>>
> >>>>>>
> >>>>>> ping??
> >>>>>>
> >>>>>> I really am for dropping support of dual-setup-able cache and config.
> >>>>>> However
> >>>>>> it is definitely not something minor.
> >>>>>
> >>>>> Hello.
> >>>>>
> >>>>> I'm not sure what to say to continue this discussion :\ I think that
> >>>>> the
> >>>>> ability for users to set a custom location for their cache is useful
> >>>>> and
> >>>>> good to have.
> >>>>
> >>>> for what? You can set it ia xdg variables. Its standartized behaviour.
> >>>
> >>> It's been said before, when you set xdg variables you are changing the
> >>> cache for not only icedtea-web but every other application that uses
> >>> xdg's
> >>> standard file locations.
> >>
> >> nope - yo can set variabnle per application... But thats metter of taste I
> >> see.
> >
> > Can you provide an example? I can't find any documentation for this.
> 
> VAR=val app...


Uh... What kind of example is this?

Do you mean exporting the variable before running the application? I don't expect the average user to know to do this :\

So in 1.5, 1.4, users can set the cache using itweb-settings. They update to 1.6 and they can no longer set the cache. Why? Are there any benefits to this?

> >
> >>>
> >>> If a user wants to change their cache in 1.6, they must change their
> >>> cache
> >>> for every other program? But in 1.5 you could change just icedtea-web
> >>> cache to any location. How is this a good thing to do?
> >>>
> >>>>
> >>>>    > Prior to this patch we had this feature, but now it's not working
> >>>>    > so I
> >>>>    > think a fix would be nice.
> >>>> What's wrong with allowing users to choose custom locations? We had this
> >>>> feature before already, I
> >>>> am just asking for a fix that makes it work again.
> >>>>> Otherwise, if you want to to remove this feature, a patch that removes
> >>>>> it
> >>>>> all would be nice. Just leaving it broken is the main issue for me.
> >>>>
> >>>> Sure. I will fix it or remove it. this+localizations is main blocker for
> >>>> release. Also Lukas's fix
> >>>> for attributes is awaited.
> >>>>>
> >>>>>>
> >>>>>> The resurrection of setupable paths may be tricky, but should be
> >>>>>> doable.
> >>>>>> Stil
> >>>>>> it do not gave sense for me....
> >>>>>
> >>>>> CacheUtil tests will test the caching system and will deal with the
> >>>>> cache
> >>>>> on the filesystem. I think it makes sense to use a temporary cache
> >>>>> location to do this.
> >>>>> ResourceTracker/ResourceDownloader/PluginBridge/other tests that touch
> >>>>> resources in the cache will deal with the cache on the filesystem. I
> >>>>> think
> >>>>> it also makes sense to use a temporary cache location to do this.
> >>>>>
> >>>>> The main reason for setupable paths is for these tests.
> >>>>
> >>>>
> >>>> yes. But those test can now use fake instance of the singleton, or not?
> >>>
> >>> At the moment, no.
> >>>
> >>> An example is CacheUtil.clearCache:.
> >>>
> >>>       public static boolean clearCache() {
> >>>           [...]
> >>>           CacheLRUWrapper lruHandler = CacheLRUWrapper.getInstance();
> >>
> >> CacheLRUWrapper lruHandler = new CacheLRUWrapper.(paths1,path2)
> >
> > Uh... what? The clearCache code contains the call to
> > CacheLRUWrapper.getInstance()... you can't just replace the code with a
> > different call.
> 
> No. Bud you can substitue fake object.

I'm sorry but that does not seem possible to me. Show me some code. I don't think you can do this with the current implementation.

> >
> >>
> >>>           [...]
> >>>
> >>> How can you test this function with a temporary cache? Otherwise, as soon
> >>> as you test it by calling CacheUtil.clearCache(), you delete the user
> >>> cache. The instance is acquired inside, and getInstance() defaults to the
> >>> one using IT-W's cache.
> >>>
> >> The issue with clearing sounds like an bug. That logic should be probably
> >> encapsulated somewhere else.
> >
> > This is just one example. CacheUtil provides utility functions for the
> > cache.
>  > Nearly every function in this class has the same issue as clearCache when
>  > it
>  > comes to writing a test that modifies a temporary cache instead of the
>  > user's current cache.
> >
>   Then it is even more buggy then I expected and have to be fixed

It was fixed by my patch [rfc][icedtea-web] Allow cache directory to be changed during runtime.

> 
> >>> A lot of changes will need to be made to inject fake instances here, but
> >>> you also have to consider things like
> >>> ResourceDownloader.downloadResource,
> >>> and making sure they can use temporary caches too, instead of downloading
> >>> into user cache.
> >>
> >> In thsi case I would say
> >>>
> >> @beforeclass
> >> original = CacheLRUWrapper.getInsatnce()
> >> CacheLRUWrapper.setInsatnce(new CacheLRUWrapper(your desired patsh again))
> >> ?
> >>
> >> @afterclass
> >> CacheLRUWrapper.setInsatnce(original)
> >
> > CacheLRUWrapper.setInstance(...)? So, now you have a setter/getter for the
> > entire instance,
> > how is this any better than using a setter/getter for the cache directory
> > itself?
> 
> I do not care how it will be done, but what must be kept:

So why did you reject the previous attempt at this?

> 
> changed variable (in your case cache/config path) on one place - everything
> will get this value (path)
>    .. This afaik was not possible with previous implementation. Nor is done

This was definitely already done in the patch I posted: [rfc][icedtea-web] Allow cache directory to be changed during runtime

>    ..
> 
> previous impl was full of hardcoded or copied values. Current impl is - yes -
> not much better.
> But at least the issue is visible.
> 
> So step one - make everything using the only (and accurate) source of truth
> Step two , make it modifiable

Once again, both step one and two were achieved in [rfc][icedtea-web] Allow cache directory to be changed during runtime.

Please consider re-reviewing the changes there. What you say you need here was already achieved there, without breaking functionality.

Please, please in the future don't rush patches like this and push them without full review. Now the situation is even more complicated than before.


Regards,

> 
> Dos not metter what instance will be injected then...
> 
> 
> Thanx for ideas!
> 
> J.
> 
> >
> >
> > Regards,
> >
> >>
> >> ?
> >>> Then you consider, how much effort you need to go through to change all
> >>> this instead of 1 getter/setter. Is it worth it?
> >>>
> >>>
> >>> Regards,
> >>>
> >>>>>
> >>>>> Thoughts? I am open to alternatives;
> >>>>>
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>>>>
> >>>>>>> J.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>> 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.
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >>
> >
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern