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

[Jie Kang]

----- Original Message -----
> On 03/20/2015 01:47 PM, Jie Kang wrote:
> >
> >
> > ----- Original Message -----
> >> On 03/19/2015 09:32 PM, Jie Kang wrote:
> >>>
> >>>
> >>> ----- Original Message -----
> >>>> On 03/04/2015 06:13 PM, Jiri Vanek wrote:
> >>>>>>>
> >>>>>>> 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!
> >>>>>
> >>>>> 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 desirable?
> >>>>
> >>>>
> >>>> 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.

> >
> > 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.

> 
> >          [...]
> >
> > 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.

> > 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? 


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