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

[Jacob Wisor]

Jiri Vanek wrote:
> On 09/05/2013 03:10 PM, Andrew Azores wrote:
>> On 09/05/13 05:28, Jacob Wisor wrote:
>>> Jiri Vanek wrote:
>>>> On 09/04/2013 07:49 PM, aazores at icedtea.classpath.org wrote:
>>>>> +    static {
>>>>> +        try {
>>>>> +            messageBundle =
>>>>> +                new
>>>>> PropertyResourceBundle(CacheReproducerTest.class.getClassLoader().getResourceAsStream(messageResourcePath)); 
>>>>>
>>>>>
>>>>> +        } catch (IOException e) {
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>
>>>> Pleae NEVER consume an exception.
>>>>
>>>> In this case the
>>>>
>>>> } catch (IOException ex) {
>>>>                 throw new RuntimeException(ex);
>>>>                }
>>>
>>> Throwing a chained RuntimeException is going to disrupt the program's 
>>> flow. So, I would rather advise
>>>
>>>         } catch (IOException e) {
>>>             e.printStackTrace();
>>>         }
>>>
>>> This will make IOException at least visible on stderr and preserve 
>>> what has been previously
>>> intended, namely continuing program execution undisrupted.
>>>
>>>> Would be the best. You can push this without any other review 
>>>> (unless you think about different fix)
>>>
>>> Regards,
>>> Jacob
>>
>> 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.
> 
> 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)

Yes, this is what I have actually meant. Just do some output at least. I was 
unaware of ServerAccess. ;-) But, Jiri may be right if the code that is supposed 
to throw an exception is critical to the test, so halting the test with an error 
value (System.exit(1)) or a chained RuntimeException may be appropriate indeed.

> 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 hard coded "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 improve it a bit more for localized messages too, and then 
> make localization reproducers/unittests much thinner. But this is even 
> another chapter.
> 
> Anyway I do not insists.

Regards,
Jacob