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

[Kirill Zhaldybin]

Marcus,

Here are a new webrev: 
http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8169003/webrev.01/

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
>>>>>
>>>>
>>>
>>
>