/hg/icedtea-web: Fixed "could not clear cache" message and cache...

[Jiri Vanek]

..snip..
>>>>
>>>>
>>>>
>>>
>>> I do like this idea, and I whipped up a quick utility class for it just now. But a side effect of
>>> extracting this logic out into a helper class is that the IOException thrown if the
>>> Messages.properties file(s) are not available is now only thrown when the new utility class is
>>> used. So in the case of this CacheReproducer, it only happens in the clearCacheUnsuccessfully
>>> test method. If something goes wrong in the utility class, eg Messages.properties is not
>>> available, then only this test fails. If we want the "stop the whole reproducer" behaviour we'll
>>> need to use static blocks or @BeforeClass annotated methods or something to force checking if the
>>> resources are available.
>>>
>>> Thanks,
>>>
>>
>> I would consider it as ok :)
>>
>> if you implement properties  singleton like this:
>>
>>
>> getSingleton(){
>> if (singleton=null) then loadSingleton()
>> }
>>
>> getMessage(key){
>> return getSingleton().getMessage(key)
>> }
>>
>>
>> Then relevant exception will be thrown during each use. So I guess the behaviour will be most
>> correct.
>>
>> Thanx for stitching with it!
>>
>> J.
>>
>
> Changelog:
> * tests/test-extensions/net/sourceforge/jnlp/tools/MessageProperties.java: new helper class for
> retrieving values from Messages.properties
> * tests/reproducers/signed/CacheReproducer/testcases/CacheReproducerTest.java: refactor to use new
> MessageProperties helper class
>
> This'll throw an exception whenever you call #getMessage, so individual test methods using this will
> fail if the resource is not available, but the whole reproducer won't halt.
>
> Thanks,
>
> --
> Andrew A
>
>
> CacheReproducer2.patch
>
>
> diff --git a/tests/reproducers/signed/CacheReproducer/testcases/CacheReproducerTest.java b/tests/reproducers/signed/CacheReproducer/testcases/CacheReproducerTest.java
> --- a/tests/reproducers/signed/CacheReproducer/testcases/CacheReproducerTest.java
> +++ b/tests/reproducers/signed/CacheReproducer/testcases/CacheReproducerTest.java
> @@ -51,6 +51,7 @@
>   import net.sourceforge.jnlp.ProcessResult;
>   import net.sourceforge.jnlp.annotations.KnownToFail;
>   import net.sourceforge.jnlp.config.Defaults;
> +import net.sourceforge.jnlp.tools.MessageProperties;
>   import org.junit.AfterClass;
>   import org.junit.Assert;
>
> @@ -77,17 +78,6 @@
>               "locks" + File.separator +
>               "netx_running");
>
> -    private static final String messageResourcePath = "net/sourceforge/jnlp/resources/Messages.properties";
> -    private static PropertyResourceBundle messageBundle = null;
> -
> -    static {
> -        try {
> -            messageBundle =
> -                new PropertyResourceBundle(CacheReproducerTest.class.getClassLoader().getResourceAsStream(messageResourcePath));
> -        } catch (IOException e) {
> -        }
> -    }
> -
>       String testS = "#netx file\n"
>                  + "#Mon Dec 12 16:20:46 CET 2011\n"
>                  + "1323703236508,0=/home/xp13/.icedtea/cache/0/http/localhost/ReadPropertiesBySignedHack.jnlp\n"
> @@ -284,7 +274,7 @@
>           Thread.sleep(1000);
>           pr = tryToClearcache();
>
> -        String cacheClearError = messageBundle.getString("CCannotClearCache");

This is incorrect. The message do not need to be en. Although most of us have english locales, the 
framework should solve this.

> +        String cacheClearError = MessageProperties.getMessage_en("CCannotClearCache");
>           Assert.assertTrue("Stderr should contain " + cacheClearError + ", but did not.", pr.stderr.contains(cacheClearError));
>           assertCacheIsNotEmpty();
>       }
> @@ -325,7 +315,6 @@
>       }
>
>       @Test
> -    @KnownToFail
>       public void testAlreadyLoadedCached11() throws Exception {
>           testsBody(CR11, 1);
>           testsBody(CR11, 2);
> diff --git a/tests/test-extensions/net/sourceforge/jnlp/tools/MessageProperties.java b/tests/test-extensions/net/sourceforge/jnlp/tools/MessageProperties.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-extensions/net/sourceforge/jnlp/tools/MessageProperties.java
> @@ -0,0 +1,79 @@
> +package net.sourceforge.jnlp.tools;
> +
> +import java.util.PropertyResourceBundle;
> +import java.io.IOException;
> +
> +public class MessageProperties {

I like this enum!

> +
> +    public enum Locale {
> +        en,
> +        de,
> +        cs,
> +        pl
> +    }
> +
> +    private static final String resourcePath = "net/sourceforge/jnlp/resources/Messages";
> +    private static final String resourceExtension = ".properties";

I would like see the loaded resources as singletons, and so avoid repeated loading. I would say 
static final Map<Locale, PropertyResourceBundle> can be enough.
> +
> +    /* eg if given locale "de", returns a PropertyResourceBundle using
> +     * "net/sourceforge/jnlp/resources/Messages_de.properties".
> +     * if given "en", the same but using "net/sourceforge/jnlp/resources/Messages.properties"
> +     */
> +    private static PropertyResourceBundle getMessageBundleFromLocale(Locale locale) throws IOException {
> +        final ClassLoader cl = MessageProperties.class.getClassLoader();
> +
> +        final String localizationCode;
> +        if (locale == Locale.en) {
> +            localizationCode = "";
> +        } else {
> +            localizationCode = "_" + locale.toString();
> +        }
> +
> +        final String path = resourcePath + localizationCode + resourceExtension;
> +        return new PropertyResourceBundle(cl.getResourceAsStream(path));
> +    }
> +
> +    /**
> +     * Retrieve a localized message from resource file
> +     * @param locale the localization of Messages.properties to search
> +     * @param key
> +     * @return the message corresponding to the given key from the specified localization
> +     * @throws IOException if the specified Messages localization is unavailable
> +     */

The base method for this should be

  public static String getMessage(String key) throws IOException
which will determine the locale and the call
  getMessage(Locale locale, String key) throws IOException {

Also I think I would abandon the  getMessage_XXX as they are easily to be used via your enum.
But as you already wrote them.. :)


And sorry to dig it - this class need unittest.. :(

> +    public static String getMessage(Locale locale, String key) throws IOException {
> +        return getMessageBundleFromLocale(locale).getString(key);
> +    }
> +
> +    /**
> +     * Convenience method, same as {@link #getMessage(Locale, String)} with the locale
> +     * set to "en"
> +     */
> +    public static String getMessage_en(String key) throws IOException {
> +        return getMessage(Locale.en, key);
> +    }
> +
> +    /**
> +     * Convenience method, same as {@link #getMessage(Locale, String)} with the locale
> +     * set to "de"
> +     */
> +    public static String getMessage_de(String key) throws IOException {
> +        return getMessage(Locale.de, key);
> +    }
> +
> +    /**
> +     * Convenience method, same as {@link #getMessage(Locale, String)} with the locale
> +     * set to "cs"
> +     */
> +    public static String getMessage_cs(String key) throws IOException {
> +        return getMessage(Locale.cs, key);
> +    }
> +
> +    /**
> +     * Convenience method, same as {@link #getMessage(Locale, String)} with the locale
> +     * set to "pl"
> +     */
> +    public static String getMessage_pl(String key) throws IOException {
> +        return getMessage(Locale.pl, key);
> +    }
> +
> +}
>