Test coverage for FrameLoader

[Adam Barth-3]

The usual WebKit policy is not to accept functional changes without
regression tests.  I'd like to ask that this policy be enforced
strictly for FrameLoader.  For example, I don't think we should be
taking changes like this without tests:

https://bugs.webkit.org/show_bug.cgi?id=30573

This policy is going to block fixing some FrameLoader bugs until we
creating a better loading test harness, but I think the long-term
benefits will out-weigh the short term costs.

Adam
_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[Brady Eidson]

Absolutely.

~Brady

On Oct 20, 2009, at 12:50 PM, Adam Barth wrote:

> The usual WebKit policy is not to accept functional changes without
> regression tests.  I'd like to ask that this policy be enforced
> strictly for FrameLoader.  For example, I don't think we should be
> taking changes like this without tests:
>
> https://bugs.webkit.org/show_bug.cgi?id=30573
>
> This policy is going to block fixing some FrameLoader bugs until we
> creating a better loading test harness, but I think the long-term
> benefits will out-weigh the short term costs.
>
> Adam
> _______________________________________________
> webkit-dev mailing list
> webkit-dev@...
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[Maciej Stachowiak]

On Oct 20, 2009, at 12:50 PM, Adam Barth wrote:

> The usual WebKit policy is not to accept functional changes without
> regression tests.  I'd like to ask that this policy be enforced
> strictly for FrameLoader.  For example, I don't think we should be
> taking changes like this without tests:
>
> https://bugs.webkit.org/show_bug.cgi?id=30573
>
> This policy is going to block fixing some FrameLoader bugs until we
> creating a better loading test harness, but I think the long-term
> benefits will out-weigh the short term costs.

To clarify, we haven't been 100% strict about this in the past for  
areas of the code where we don't have the proper test infrastructure.  
That being said, it's probably time to build more infrastructure for  
testing page loading properly. Also, back/forward specifically can  
already be tested in DumpRenderTree, albeit somewhat awkwardly  
(assuming the symptom of this bug will manifest just by going back and  
forward).

  - Maciej

_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[tonikitoo (Antonio Gomes)]

Hi,

As I replied in the bug, i totally agree about patch needing a test,
and more generically that Test Coverage is something to be improved
(not to get worse).

Talking in my own defense, (again) i knew patch was not yet ready to
get in (no tests) but since I do not have a MAC box handy and then can
not run current MAC layout tests, I actually requested for comments
from people i know touched the code lately. I was even hoping that
maybe to some one w/ a Mac to run the test batch w/ and w/o the patch
to see if it breaks what is already there, however i did not
explicitly expressed my desire in the bug and ... So Adam, please do
not take me wrong by requesting review for this
not-yet-ready-to-be-reviewed patch. I believe almost every body
already did what i did :-)

/me would love try servers to be available.

On Tue, Oct 20, 2009 at 4:26 PM, Maciej Stachowiak <mjs@...> wrote:

>
> On Oct 20, 2009, at 12:50 PM, Adam Barth wrote:
>
>> The usual WebKit policy is not to accept functional changes without
>> regression tests.  I'd like to ask that this policy be enforced
>> strictly for FrameLoader.  For example, I don't think we should be
>> taking changes like this without tests:
>>
>> https://bugs.webkit.org/show_bug.cgi?id=30573
>>
>> This policy is going to block fixing some FrameLoader bugs until we
>> creating a better loading test harness, but I think the long-term
>> benefits will out-weigh the short term costs.
>
> To clarify, we haven't been 100% strict about this in the past for areas of
> the code where we don't have the proper test infrastructure. That being
> said, it's probably time to build more infrastructure for testing page
> loading properly. Also, back/forward specifically can already be tested in
> DumpRenderTree, albeit somewhat awkwardly (assuming the symptom of this bug
> will manifest just by going back and forward).
>
>  - Maciej
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev@...
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>

--
--Antonio Gomes
_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[Adam Barth-3]

On Tue, Oct 20, 2009 at 1:34 PM, tonikitoo (Antonio Gomes)
<tonikitoo@...> wrote:
> Talking in my own defense,

Oh, I didn't mean to cast dispersions on your patch in particular.
This issue has come up a couple times and sometimes a FrameLoader
patch gets through without tests because we don't have a good testing
harness for loading.  I wanted to raise the issue to make sure we're
in agreement on being stricter for a while.

Adam
_______________________________________________
webkit-dev mailing list
webkit-dev@...
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev