[icedtea-web] RFC: Fix pac tests
jvanek at redhat.com
Tue Mar 27 00:16:22 PDT 2012
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;
>>> 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 :)
More information about the distro-pkg-dev