RFR(XS): 8169003: LogDecorations.iso8601_utctime_test fails if numeric locale uses ", " as separator between integer and fraction part

[Marcus Larsson]

Hi,


On 2016-11-21 17:38, Kirill Zhaldybin wrote:
> Marcus,
>
> Thank you for reviewing the fix!
>>> WebRev: 
>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8169003/webrev.00/
>>
>> ISO8601 says the decimal point can be either '.' or ',' so the test 
>> should accept either. You could let sscanf read out the decimal point 
>> as a character and just verify that it is one of the two.
>>
>> In the UTC test you changed ASSERT_GT to ASSERT_EQ, which means that 
>> we won't accept "Z" suffixed strings. Please revert that.
> I agree that ISO8601 could add "Z" to time (and as far as I understand 
> date/time without delimiters is legal too) but these are the unit tests.
> Hence they cover the existing code and they should pass only if the 
> result corresponds to existing code and fail otherwise.
> The current code from os::iso8601_time format date/time string 
> %04d-%02d-%02dT%02d:%02d:%02d.%03d%c%02d%02d so we should not consider 
> any other format as valid.
>
> Could you please let me know your opinion?

I think the test should verify the intended behavior, not the 
implementation. If we refactor or change something in iso8601_time() we 
shouldn't be failing the test if it still conforms to ISO8601, IMO.

Thanks,
Marcus

>
> Thank you.
>
> Regards, Kirill
>
>>
>> Thanks,
>> Marcus
>>
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8169003
>>>
>>> Thank you.
>>>
>>> Regards, Kirill
>>
>