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

[Marcus Larsson]

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