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

[Jiri Vanek]

On 09/06/2013 04:40 PM, Andrew Azores wrote:
> 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,
>

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.