[icedtea-web] RFC: Fix pac tests

[Jiri Vanek]

On 03/26/2012 07:55 PM, Omair Majid wrote:
> Hi Jiri,
>
> On 03/26/2012 01:31 PM, Jiri Vanek wrote:
>> On 03/24/2012 08:13 AM, Omair Majid wrote:
>>>
>>> The attached patch fixes the pac tests. With the patch applied, the
>>> results change from:
>>>
>>> Test results: passed: 220; failed: 64;
>>>
>>> to:
>>>
>>> Test results: passed: 285; failed: 0;
>>>
>>> I have rewritten the testDateRange* tests so date wrapping is handled
>>> correctly. I also discovered two bugs in pac-funcs.js (yes, tests help
>>> find bugs!) and I have fixed those too.
>>>
>> Hi! The logic itself looks ok. The fullYear() fix can go inside
>> immediately (as separate patch)
>
> Thanks for the review!
>
>> But fixes inside /tests/netx/pac/pac-funcs-test.js contains both
>> cosmetic and  functional fixes. Can you please separate them? (I think
>> that cosmetic ones can go inside imidietly without nay more reviewing)
>
> I am not entirely clear on what you mean. I haven't added or removed any
> tests. The test changes really are all cosmetic. As you know, the test
> were not able to handle day/month wraps and some tests would fail on the
> start or end of the month. The tests now handle these corner cases
> properly so they should behave the same on any given day. The actual
> cases they are testing (things like "is today within the date range
> yesterday-tomorrow?") has not been changed. Unless I made a mistake,
> that is. Please let me know if you see any such cases.

no no nothing like this:)
Among the wrapping and other fixing you did are several times just fixes of indentation.
Do you mind to push those fixes separately and repost the fixes you did again?
>
>> /me hopes not to make you angry to much :(
>>
>
> How would a patch review make me angry? I am going to thank you instead
> for raising your concerns!

Well I'm the last one who should do some criticism with indentations a co. But this particular changes made the patch strange to read.
Thats why I apologised O:)
>
>> When I was checking this issue I wanted to fix it, but now when I have
>> seen your change-set  I'm very happy I did not so. Tyvm for very deep fix!
>
> No problem :)
>
> Cheers,
> Omair

J.