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

[Jie Kang]

----- Original Message -----
> On 03/03/2015 04:25 PM, Jie Kang wrote:
> >
> >
> > ----- Original Message -----
> >> snip
> >>> However. Later I noted that recently_used file is not handled via
> >>> PahsAndFiles.
> >>>
> >>> I moved it here, which needed some more changes - like make it properly
> >>> testable and so get rid of
> >>> this untestable ENUM and replace it by getter singleton with possibility
> >>> to
> >>> make testable instance.
> >>>
> >>> I agree that this patch was written in rush, and needs  proper review.
> >>> J.
> >
> > Hello,
> >
> > Nits below:
> >
> > +    public final PropertiesFile cacheOrder;
> > +    public final File cacheOrderParent;
> > +    public final File cacheOrderFile;
> >
> > The name 'cacheOrder' isn't very descriptive. Can you change it to
> > recentlyUsedFile or something similar?
> >
> > Can you use getters instead of public variables? Then you can reduce it to
> > just one variable.
> >
> 
> Done. Now the patch is a bit dirty, but it was good change.
> 
> > e.g.
> >
> > private final PropertiesFile recentlyUsed;
> >
> > public PropertiesFile getProperties() { ... }
> > public File getParentFile() { ... }
> > public File getFile() { ... }
> >
> > Names of function are up to you.

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


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




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)



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


Regards,

> >
> > +    private static final CacheLRUWrapper clw = new CacheLRUWrapper(new
> > File(System.getProperty("java.io.tmpdir"),cacheIndexFileName));
> >
> > Space after the comma: ("java.io.tmpdir"),cacheIndexFileName
> done
> >
> > Also, this will set the cache to the tmp directory. Can you create a
> > tempcache directory inside tmp and then use that instead?
> 
> I hate how this is done in java... anyweay done.
> >
> > It's a little bit cleaner. And please make sure the tests clean themselves
> > up afterwards using file.deleteOnExit(), etc.
> done
> >
> > +    private static final int PERNAMENT_FILES = 1;
> >
> > s/PERNAMENT/PERMANENT
> 
> Thanx!
> >
> >
> > Side note:
> >
> > I don't see how this is more testable than before. enum singleton and class
> > singleton are equally testable to me. Can you provide any examples?
> >
> 
> To beable to create fake testable instance.
> >
> > Regards,
> >
> >>
> >> ping
> >>
> >
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern