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

[Kirill Zhaldybin]

Markus,

Thank you for review!

Regards, Kirill

On 28.11.2016 13:06, Marcus Larsson 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>