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

[Andrew Azores]

On 09/06/2013 03:11 AM, Jiri Vanek wrote:
> On 09/05/2013 03:10 PM, Andrew Azores wrote:
>> (snip)
>> As Jacob says, I'd rather not throw a chained exception here because 
>> that would cause the entire
>> reproducer to halt, would it not? But if the messageBundle is not 
>> properly instantiated that's only
>> really a blocker for one test (clearCacheUnsucessfully). This 
>> exception being consumed would
>> manifest as a NPE in that later test, which would just result in that 
>> single test failing. Printing
>> the stack trace is agreeable enough but I don't really think the 
>> entire reproducer needs to stop if
>> we're going to be unable to run one test.
>>
>
>
> imho nope. If messages.properties would become unavailable, then it is 
> so crucial that I would be happy even for whole test run failure.

Hm yea, I suppose that's true. However, see below -

>
> Also not
> >>         } catch (IOException e) {
> >>             e.printStackTrace();
> >>         }
>
> but
>
> >>         } catch (IOException e) {
>  ServerAccess.logException(ex);
> >>         }
>
> (never use stdout/err in testcases, but always appropriate 
> ServerAccess.log* function)
>
>
>
> In this particular case do as you wish. My recommendation would lead 
> to separate  changeset, where you will create static method 
> getMessage(String key) somewhere in 
> test-extensions/net/sourceforge/jnlp/tools (probably new utility 
> class) which will encapsulate  the
> >>>> +    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) {
>
> with throw here :)
>
> >>>> +        }
> >>>> +    }
> and
> >>>> +        String cacheClearError = 
> messageBundle.getString("CCannotClearCache");
>
>
> parts.
>
> The reason for this is, that the hardocded "evaluate string" is wide 
> spread issue, and I believe some of the failures are because of small 
> change in properties file (eg the "the"  removal/addition or something 
> like that)
>
> Also you can imprve it a bit more for localised messages too, and then 
> make localisation reproducers/unittests much thinner. But this is even 
> another chapter.
>
> Anyway I do not insists.
>
>
>
>

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,

-- 
Andrew A