[icedtea-web] RFC: Fix pac tests

Omair Majid omajid at redhat.com
Mon Mar 26 10:55:51 PDT 2012


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.

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

> 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



More information about the distro-pkg-dev mailing list