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

Marcus Larsson marcus.larsson at oracle.com
Thu Nov 24 14:35:37 UTC 2016


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



More information about the hotspot-runtime-dev mailing list