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

[Igor Ignatyev]

Hi Kirill,

looks good to me, thanks for fixing that. 

Cheers,
— Igor

> On Nov 28, 2016, at 1:06 PM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
> 
> Hi,
> 
> 
> On 11/25/2016 06:23 PM, Kirill Zhaldybin wrote:
>> Marcus,
>> 
>> Here are a new webrev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8169003/webrev.01/
> 
> Looks ok.
> 
> Thanks,
> Marcus
> 
>> 
>> I addressed your comment about "if-case for the Z suffix".
>> 
>> Could you please let me know your opinion?
>> 
>> Thank you.
>> 
>> Regards, Kirill
>> 
>> On 24.11.2016 17:35, Marcus Larsson wrote:
>>> Hi,
>>> 
>>> 
>>> On 11/22/2016 02:24 PM, Kirill Zhaldybin wrote:
>>>> 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.
>>> 
>>> I still think it's unnecessary noise, but if you insist I'm fine with it.
>>> 
>>> If we're not going to accept anything else than the current
>>> implementation then you should also remove the if-case for the Z suffix,
>>> since the test will fail for that anyway.
>>> 
>>> Thanks,
>>> Marcus
>>> 
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Marcus
>>>>> 
>>>>>> 
>>>>>> Thank you.
>>>>>> 
>>>>>> Regards, Kirill
>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Marcus
>>>>>>> 
>>>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8169003
>>>>>>>> 
>>>>>>>> Thank you.
>>>>>>>> 
>>>>>>>> Regards, Kirill
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>