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

[Jie Kang]

----- Original Message -----
> On 03/04/2015 04:01 PM, Jie Kang wrote:
> >
> >
> > ----- Original Message -----
> >> snip
> >>>
> >>> +    private final PropertiesFile recentlyUsedPropertiesFile;
> >>> +    private final File cacheDir;
> >>> +    private final File recentlyUsedFile;
> >>>
> >>> +    /**
> >>> +     * @return the recentlyUsedFile
> >>> +     */
> >>> +    public File getRecentlyUsedFile() {
> >>> +        return recentlyUsedFile;
> >>> +    }
> >>>
> >>> Instead of keeping recentlyUsedFile in memory, can you change it to:
> >>>
> >>> +    public File getRecentlyUsedFile() {
> >>> +        return recentlyUsedPropertiesFile.getStoreFile();
> >>> +    }
> >>>
> >>> Then you can remove that variable.
> >>
> >> As    you wish, done.
> >>>
> >>>
> >>> -        if (cacheOrder.containsKey(key)) {
> >>> +        if (getRecentlyUsedPropertiesFile().containsKey(key)) {
> >>>
> >>> AFAICT this code is inside CacheLRUWrapper so it should have access to
> >>> 'recentlyUsedPropertiesFile'. It shouldn't use the getter if it doesn't
> >>> need to, no?
> >>>
> >>> A lot of the code changes are like this. Can you change them all back?
> >>
> >> With the getter you wished no. And I vote for keeping the getter rather
> >>>
> >>>
> >>>
> >>>
> >>> Also:
> >>>
> >>> +    public CacheLRUWrapper() {
> >>> +     this(PathsAndFiles.RECENTLY_USED_FILE.getFile());
> >>> +    }
> >>>
> >>> In terms of the constructor, the dependencies are opposite.
> >>>
> >>> Right now, you get recently_used file, and then get cacheDir off it.
> >>>
> >>> Instead, it should be get cacheDir, and then get recently_used file off
> >>> it.
> >>>
> >>> E.g.
> >>>
> >>> RecentlyUsedFile is always inside the Cache.  (this is correct)
> >>>
> >>> as opposed to
> >>>
> >>> Cache is always parent file of RecentlyUsedFile. (In most cases this ends
> >>> up being the same thing, but it's not as correct)
> >>
> >> Right. I removed misleading single-file constructor.
> >>>
> >>>
> >>>
> >>> The default constructor usage ends up breaking things when you use it in
> >>> CacheUtil.
> >>>
> >>>       private static final String setCacheDir =
> >>>       JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR);
> >>>       private static final String cacheDir = new File(setCacheDir != null
> >>>       ?
> >>>       setCacheDir : System.getProperty("java.io.tmpdir")).getPath(); //
> >>>       Do
> >>>       this with file to standardize it.
> >>>       private static final CacheLRUWrapper lruHandler =
> >>>       CacheLRUWrapper.getInstance();
> >>>
> >>>    You need to also add changes to CacheUtil to use lruHandler's cache
> >>>    directory.
> >>>
> >>>    At the moment, CacheUtil.cacheDir != lruHandler.getCacheDir() when
> >>>    someone sets DeploymentConfiguration.KEY_USER_CACHE_DIR to
> >>>    non-default.
> >>
> >> Yes. Fixed.
> >>>
> >>>
> >>> This patch is broken here and needs to be fixed.
> >>>
> >>>
> >>> Why would you push this without a +1 from anyone? You also seem to be
> >>> doing
> >>> this with a number of other patches recently. If you need a review, ping
> >>> someone.
> >>
> >> :( Rush is an eneymy. I need to calm bit down with all those failures...
> >>
> >>
> >> See the patch for this commit.
> >
> > Hello,
> >
> >
> > -    private static final CacheLRUWrapper lruHandler =
> > CacheLRUWrapper.getInstance();
> >
> > I feel like keeping a static instance here is still okay... Does it cause
> > any problems?
> 
> Unless you insists, I will go with the field removed.
> 
> >
> > When the code inside CacheUtil calls: CacheLRUWrapper.getInstance() : it's
> > receiving the same static instance anyways. So you should be able to just
> > call it once and keep it for future use without any problems.
> >
> >
> > Without the static instance you might want to move some code around to be
> > more clean.
> >
> > E.g:
> >
> > @@ -145,12 +143,13 @@
> >               return false;
> >           }
> >
> > -        File cacheDir = new File(CacheUtil.cacheDir);
> > +        File cacheDir = CacheLRUWrapper.getInstance().getCacheDir();
> >           if (!(cacheDir.isDirectory())) {
> >               return false;
> >           }
> >
> >           OutputController.getLogger().log(OutputController.Level.ERROR_DEBUG,
> >           "Clearing cache directory: " + cacheDir);
> > +        CacheLRUWrapper lruHandler = CacheLRUWrapper.getInstance();
> >
> > You should get the instance once before the line "File cacheDir =
> > CacheLRUWrapper.getInstance().getCacheDir();", and reuse it throughout the
> > method. This applies to a lot of the methods now.
> >
> >
> > The rest looks good.
> >
> > The tests are all passing for me with this patch applied. For some reason
> > though, I feel like there are still some hidden bugs in the caching system
> > :\ I'll look into it.
> >
> >
> Yes. The caching system had a lot of bugs in past. Mostl caused by various
> duplicated providers.
> 
> Ok to push  now?

Sure.


Regards,

> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern