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

[Jie Kang]

----- 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());
     }

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.

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

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.


Regards,

> 
> ty!
> 
> J.
> >
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern