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

[Kirill Zhaldybin]

Marcus,

Thank you for prompt reply!

Could you please read comments inline?
I'm looking forward to your reply.

Thank you.

Regards, Kirill

On 22.11.2016 15:32, Marcus Larsson wrote:
> 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.
I would agree with you if we were talking about a functional test. But 
since it is an unit test I think we should keep it as close to 
implementation as possible.
If the implementation is changed unintentionally the test fails and 
signals us that something is broken.
If it is an intentional change the test must be updated correspondingly.

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