Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7104081:
Browser Test (Prerendering) for Favicon (Closed)
|
Created:
9 years, 6 months ago by tburkard Modified:
9 years, 6 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionBrowser Test to verify that a favicon is being displayed correctly after a prerendered page is swapped in.
R=dominich@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88527
Patch Set 1 #
Total comments: 3
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 2
Messages
Total messages: 13 (0 generated)
Hi Dominic, ok, so I redid the change with Shishir's new option, and I also cleaned up his code a bit. Thanks. :) Timo
Drive-by with test comments. http://codereview.chromium.org/7104081/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/7104081/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_browsertest.cc:1355: 2000); No hardcoded timeouts please. Could you use base/test/test_timeouts? That will automagically save you trouble with Valgrind. By the way, it would be much better to wait for the favicon to become valid than to quit the message loop after some fixed timeout.
http://codereview.chromium.org/7104081/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/7104081/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_browsertest.cc:1355: 2000); On 2011/06/09 09:45:21, Paweł Hajdan Jr. wrote: > No hardcoded timeouts please. Could you use base/test/test_timeouts? That will > automagically save you trouble with Valgrind. > > By the way, it would be much better to wait for the favicon to become valid than > to quit the message loop after some fixed timeout. Done.
http://codereview.chromium.org/7104081/diff/1/chrome/browser/prerender/preren... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/7104081/diff/1/chrome/browser/prerender/preren... chrome/browser/prerender/prerender_browsertest.cc:1355: 2000); On 2011/06/09 09:51:16, tburkard wrote: > On 2011/06/09 09:45:21, Paweł Hajdan Jr. wrote: > > No hardcoded timeouts please. Could you use base/test/test_timeouts? That will > > automagically save you trouble with Valgrind. > > > > By the way, it would be much better to wait for the favicon to become valid > than > > to quit the message loop after some fixed timeout. > > Done. How about waiting for the favicon to become valid? A timeout from TestTimeouts is better than a hardcoded one, but still always waiting for a fixed amount of time is a bad testing style. It's also important to keep the tests fast.
Well, what if the test fails (because the icon does not appear)? In that case, the test would time out. That would not keep the test fast. In addition to that, if the favicon does not show up within 2 seconds, something else is seriously wrong, so it'd be good to get someone's attention. Timo On Thu, Jun 9, 2011 at 2:55 AM, <phajdan.jr@chromium.org> wrote: > > http://codereview.chromium.**org/7104081/diff/1/chrome/** > browser/prerender/prerender_**browsertest.cc<http://codereview.chromium.org/7104081/diff/1/chrome/browser/prerender/prerender_browsertest.cc> > File chrome/browser/prerender/**prerender_browsertest.cc (right): > > http://codereview.chromium.**org/7104081/diff/1/chrome/** > browser/prerender/prerender_**browsertest.cc#newcode1355<http://codereview.chromium.org/7104081/diff/1/chrome/browser/prerender/prerender_browsertest.cc#newcode1355> > chrome/browser/prerender/**prerender_browsertest.cc:1355: 2000); > On 2011/06/09 09:51:16, tburkard wrote: > >> On 2011/06/09 09:45:21, Paweł Hajdan Jr. wrote: >> > No hardcoded timeouts please. Could you use base/test/test_timeouts? >> > That will > >> > automagically save you trouble with Valgrind. >> > >> > By the way, it would be much better to wait for the favicon to >> > become valid > >> than >> > to quit the message loop after some fixed timeout. >> > > Done. >> > > How about waiting for the favicon to become valid? A timeout from > TestTimeouts is better than a hardcoded one, but still always waiting > for a fixed amount of time is a bad testing style. It's also important > to keep the tests fast. > > > http://codereview.chromium.**org/7104081/<http://codereview.chromium.org/7104... >
On 2011/06/09 10:33:14, tburkard wrote: > Well, what if the test fails (because the icon does not appear)? In that > case, the test would time out. That would not keep the test fast. I don't get this argument. That's why we keep the browser test timeout short (~25 s) and DISABLE tests that time out. The behavior of the test when it fails has nothing to do with the average case, and the test is expected to pass. Now the current code makes the test always wait a fixed amount of time. It is arguably faster to only wait the needed amount. Finally, from the flakiness POV, it's really better to wait for an event. If you wait a fixed amount of time, it's not obvious how to switch that code to one that waits for an event. That creates a temptation to write more tests that way, which may lead to more flakiness. > In addition to that, if the favicon does not show up within 2 seconds, > something else is seriously wrong, so it'd be good to get someone's > attention. You can't really rely on any fixed time periods in browser tests, because the variance on the bots may well exceed 2 seconds. Also, this kind of failure tends to be flaky. And then, flaky failures are not good attention-grabbers: http://code.google.com/p/chromium/issues/detail?id=80561 is there since April. Nothing happened to that bug. http://code.google.com/p/chromium/issues/detail?id=83170, nothing happened since May 18th. There's also http://code.google.com/p/chromium/issues/detail?id=83901 and http://code.google.com/p/chromium/issues/detail?id=77323 which seem to be worked on (thanks!). > Timo > > On Thu, Jun 9, 2011 at 2:55 AM, <mailto:phajdan.jr@chromium.org> wrote: > > > > > http://codereview.chromium.**org/7104081/diff/1/chrome/** > > > browser/prerender/prerender_**browsertest.cc<http://codereview.chromium.org/7104081/diff/1/chrome/browser/prerender/prerender_browsertest.cc> > > File chrome/browser/prerender/**prerender_browsertest.cc (right): > > > > http://codereview.chromium.**org/7104081/diff/1/chrome/** > > > browser/prerender/prerender_**browsertest.cc#newcode1355<http://codereview.chromium.org/7104081/diff/1/chrome/browser/prerender/prerender_browsertest.cc#newcode1355> > > chrome/browser/prerender/**prerender_browsertest.cc:1355: 2000); > > On 2011/06/09 09:51:16, tburkard wrote: > > > >> On 2011/06/09 09:45:21, Paweł Hajdan Jr. wrote: > >> > No hardcoded timeouts please. Could you use base/test/test_timeouts? > >> > > That will > > > >> > automagically save you trouble with Valgrind. > >> > > >> > By the way, it would be much better to wait for the favicon to > >> > > become valid > > > >> than > >> > to quit the message loop after some fixed timeout. > >> > > > > Done. > >> > > > > How about waiting for the favicon to become valid? A timeout from > > TestTimeouts is better than a hardcoded one, but still always waiting > > for a fixed amount of time is a bad testing style. It's also important > > to keep the tests fast. > > > > > > > http://codereview.chromium.**org/7104081/%3Chttp://codereview.chromium.org/71...> > >
LGTM Thanks for tidying up the access to the TestPrerenderContents.
In the future, please hold off on committing until there is agreement from reviewers. I think you should revisit how you test for the favicon, and have a suggestion. Pawel - we have also attacked a number of the flaky prerender tests - if you look at the closed bugs in addition to the open bugs you will see a lot of history there. For example, http://code.google.com/p/chromium/issues/list?can=1&q=Feature%3DPreload+flaky... shows only 3 out of 12 flakiness related bugs have not been resolved. http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:1353: MessageLoopForUI::current()->PostDelayedTask( I agree with Pawel that this is pretty hairy. Could you do something like the TitleWatcher in http://codereview.chromium.org/7046053/ which listens for the FAVICON_CHANGED event instead?
I ran this test many times on trybots and it was not flaky. Should displaying the favicon take e.g. 20 seconds, this would be a serious error condition that should be flagged, so it seems to me like there should be something timer-dependent? I will look into what you suggested. Thanks. Timo On Thu, Jun 9, 2011 at 8:41 AM, <cbentzel@chromium.org> wrote: > In the future, please hold off on committing until there is agreement from > reviewers. I think you should revisit how you test for the favicon, and > have a > suggestion. > > Pawel - we have also attacked a number of the flaky prerender tests - if > you > look at the closed bugs in addition to the open bugs you will see a lot of > history there. For example, > http://code.google.com/p/**chromium/issues/list?can=1&q=** > Feature%3DPreload+flaky&**colspec=ID+Stars+Pri+Area+** > Feature+Type+Status+Summary+**Modified+Owner+Mstone+OS&x=** > mstone&y=area&cells=tiles<http://code.google.com/p/chromium/issues/list?can=1&q=Feature%3DPreload+flaky&colspec=ID+Stars+Pri+Area+Feature+Type+Status+Summary+Modified+Owner+Mstone+OS&x=mstone&y=area&cells=tiles> > shows only 3 out of 12 flakiness related bugs have not been resolved. > > > http://codereview.chromium.**org/7104081/diff/7001/chrome/** > browser/prerender/prerender_**browsertest.cc<http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/prerender_browsertest.cc> > > File chrome/browser/prerender/**prerender_browsertest.cc (right): > > http://codereview.chromium.**org/7104081/diff/7001/chrome/** > browser/prerender/prerender_**browsertest.cc#newcode1353<http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/prerender_browsertest.cc#newcode1353> > chrome/browser/prerender/**prerender_browsertest.cc:1353: > MessageLoopForUI::current()->**PostDelayedTask( > I agree with Pawel that this is pretty hairy. > > Could you do something like the TitleWatcher in > http://codereview.chromium.**org/7046053/<http://codereview.chromium.org/7046... listens for the > FAVICON_CHANGED event instead? > > > http://codereview.chromium.**org/7104081/<http://codereview.chromium.org/7104... >
http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/pre... File chrome/browser/prerender/prerender_browsertest.cc (right): http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/pre... chrome/browser/prerender/prerender_browsertest.cc:1353: MessageLoopForUI::current()->PostDelayedTask( On 2011/06/09 15:41:04, cbentzel wrote: > I agree with Pawel that this is pretty hairy. > > Could you do something like the TitleWatcher in > http://codereview.chromium.org/7046053/ which listens for the FAVICON_CHANGED > event instead? Yes, that's what I'd like to see. Please do that. Also, I'm slightly annoyed that the change got committed with unresolved comments. Just a note: my comments about this code are not about flakiness, at least not directly. Waiting for a fixed amount of time and waiting for an event with a possibly similar timeout are essentially equally solid. However, waiting for a fixed amount of time is just wrong. It makes the test unnecessarily slow, and might encourage a bad test writing style. It does happen, and it does matter. When most Sleeps disappeared from the codebase, people stopped adding them to new tests (yes, I'm just watching _every_ test change, so this is all based on facts and data). This PostDelayedTask is essentially a Sleep, and that's what makes me unhappy. Finally, thank you for dealing with flaky tests - I appreciate that effort very much, and the goal of my comments is just to further increase the quality of the tests.
Pawel, I will change the test to use a similar observer-triggered detection and eliminate the timeout. I will try to send out the change today. Thanks. Timo On Thu, Jun 9, 2011 at 12:29 PM, <phajdan.jr@chromium.org> wrote: > > http://codereview.chromium.**org/7104081/diff/7001/chrome/** > browser/prerender/prerender_**browsertest.cc<http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/prerender_browsertest.cc> > File chrome/browser/prerender/**prerender_browsertest.cc (right): > > http://codereview.chromium.**org/7104081/diff/7001/chrome/** > browser/prerender/prerender_**browsertest.cc#newcode1353<http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/prerender_browsertest.cc#newcode1353> > chrome/browser/prerender/**prerender_browsertest.cc:1353: > MessageLoopForUI::current()->**PostDelayedTask( > On 2011/06/09 15:41:04, cbentzel wrote: > >> I agree with Pawel that this is pretty hairy. >> > > Could you do something like the TitleWatcher in >> http://codereview.chromium.**org/7046053/<http://codereview.chromium.org/7046... listens for the >> > FAVICON_CHANGED > >> event instead? >> > > Yes, that's what I'd like to see. Please do that. Also, I'm slightly > annoyed that the change got committed with unresolved comments. > > Just a note: my comments about this code are not about flakiness, at > least not directly. Waiting for a fixed amount of time and waiting for > an event with a possibly similar timeout are essentially equally solid. > > However, waiting for a fixed amount of time is just wrong. It makes the > test unnecessarily slow, and might encourage a bad test writing style. > It does happen, and it does matter. When most Sleeps disappeared from > the codebase, people stopped adding them to new tests (yes, I'm just > watching _every_ test change, so this is all based on facts and data). > This PostDelayedTask is essentially a Sleep, and that's what makes me > unhappy. > > Finally, thank you for dealing with flaky tests - I appreciate that > effort very much, and the goal of my comments is just to further > increase the quality of the tests. > > > http://codereview.chromium.**org/7104081/<http://codereview.chromium.org/7104... >
Pawel/Chris:
before I make the change:
I am intending on modifying ui_test_utils.{cc,h}, and turn TitleWatcher into
a GenericNotificationWatcher on a TabContents, and make a virtual =0 method
"HasInterestingNotificationBeenObserved()", and then TitleWatcher will
derive from that and override HasInterestingNotificationBeenObserved to
figure out if the title matche sthe expected one. I will then provide
another derived class FaviconWatcher that registers favicon changes, and by
overriding the HasInterestingNotificationBeenObserved, checks whether the
favicon is no valid.
Does that sound like a reasonable approach?
Or should I leave TitleWatcher alone, and duplicate the code and just make
the few changes needed for my FavIcon stuff? I figured I'd double check
with you guys first.
Thanks.
Timo
On Fri, Jun 10, 2011 at 4:50 AM, Timo Burkard <tburkard@chromium.org> wrote:
> Pawel, I will change the test to use a similar observer-triggered detection
> and eliminate the timeout. I will try to send out the change today.
>
> Thanks.
>
> Timo
>
>
> On Thu, Jun 9, 2011 at 12:29 PM, <phajdan.jr@chromium.org> wrote:
>
>>
>> http://codereview.chromium.**org/7104081/diff/7001/chrome/**
>>
browser/prerender/prerender_**browsertest.cc<http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/prerender_browsertest.cc>
>> File chrome/browser/prerender/**prerender_browsertest.cc (right):
>>
>> http://codereview.chromium.**org/7104081/diff/7001/chrome/**
>>
browser/prerender/prerender_**browsertest.cc#newcode1353<http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/prerender_browsertest.cc#newcode1353>
>> chrome/browser/prerender/**prerender_browsertest.cc:1353:
>> MessageLoopForUI::current()->**PostDelayedTask(
>> On 2011/06/09 15:41:04, cbentzel wrote:
>>
>>> I agree with Pawel that this is pretty hairy.
>>>
>>
>> Could you do something like the TitleWatcher in
>>>
http://codereview.chromium.**org/7046053/<http://codereview.chromium.org/7046...
listens for the
>>>
>> FAVICON_CHANGED
>>
>>> event instead?
>>>
>>
>> Yes, that's what I'd like to see. Please do that. Also, I'm slightly
>> annoyed that the change got committed with unresolved comments.
>>
>> Just a note: my comments about this code are not about flakiness, at
>> least not directly. Waiting for a fixed amount of time and waiting for
>> an event with a possibly similar timeout are essentially equally solid.
>>
>> However, waiting for a fixed amount of time is just wrong. It makes the
>> test unnecessarily slow, and might encourage a bad test writing style.
>> It does happen, and it does matter. When most Sleeps disappeared from
>> the codebase, people stopped adding them to new tests (yes, I'm just
>> watching _every_ test change, so this is all based on facts and data).
>> This PostDelayedTask is essentially a Sleep, and that's what makes me
>> unhappy.
>>
>> Finally, thank you for dealing with flaky tests - I appreciate that
>> effort very much, and the goal of my comments is just to further
>> increase the quality of the tests.
>>
>>
>>
http://codereview.chromium.**org/7104081/<http://codereview.chromium.org/7104...
>>
>
>
On Fri, Jun 10, 2011 at 14:20, Timo Burkard <tburkard@chromium.org> wrote: > I am intending on modifying ui_test_utils.{cc,h}, and turn TitleWatcher > into a GenericNotificationWatcher on a TabContents, and make a virtual =0 > method "HasInterestingNotificationBeenObserved()", and then TitleWatcher > will derive from that and override HasInterestingNotificationBeenObserved to > figure out if the title matche sthe expected one. This will have a lot in common with the WindowedNotificationObserver, make sure to unify that too. > I will then provide another derived class FaviconWatcher that registers > favicon changes, and by overriding the > HasInterestingNotificationBeenObserved, checks whether the favicon is no > valid. > > Does that sound like a reasonable approach? > Sure, sounds good. > Or should I leave TitleWatcher alone, and duplicate the code and just make > the few changes needed for my FavIcon stuff? I figured I'd double check > with you guys first. > I generally try to avoid duplication in testing utilities. > Thanks. > > Timo > > > On Fri, Jun 10, 2011 at 4:50 AM, Timo Burkard <tburkard@chromium.org>wrote: > >> Pawel, I will change the test to use a similar observer-triggered >> detection and eliminate the timeout. I will try to send out the change >> today. >> >> Thanks. >> >> Timo >> >> >> On Thu, Jun 9, 2011 at 12:29 PM, <phajdan.jr@chromium.org> wrote: >> >>> >>> http://codereview.chromium.**org/7104081/diff/7001/chrome/** >>> browser/prerender/prerender_**browsertest.cc<http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/prerender_browsertest.cc> >>> File chrome/browser/prerender/**prerender_browsertest.cc (right): >>> >>> http://codereview.chromium.**org/7104081/diff/7001/chrome/** >>> browser/prerender/prerender_**browsertest.cc#newcode1353<http://codereview.chromium.org/7104081/diff/7001/chrome/browser/prerender/prerender_browsertest.cc#newcode1353> >>> chrome/browser/prerender/**prerender_browsertest.cc:1353: >>> MessageLoopForUI::current()->**PostDelayedTask( >>> On 2011/06/09 15:41:04, cbentzel wrote: >>> >>>> I agree with Pawel that this is pretty hairy. >>>> >>> >>> Could you do something like the TitleWatcher in >>>> http://codereview.chromium.**org/7046053/<http://codereview.chromium.org/7046... listens for the >>>> >>> FAVICON_CHANGED >>> >>>> event instead? >>>> >>> >>> Yes, that's what I'd like to see. Please do that. Also, I'm slightly >>> annoyed that the change got committed with unresolved comments. >>> >>> Just a note: my comments about this code are not about flakiness, at >>> least not directly. Waiting for a fixed amount of time and waiting for >>> an event with a possibly similar timeout are essentially equally solid. >>> >>> However, waiting for a fixed amount of time is just wrong. It makes the >>> test unnecessarily slow, and might encourage a bad test writing style. >>> It does happen, and it does matter. When most Sleeps disappeared from >>> the codebase, people stopped adding them to new tests (yes, I'm just >>> watching _every_ test change, so this is all based on facts and data). >>> This PostDelayedTask is essentially a Sleep, and that's what makes me >>> unhappy. >>> >>> Finally, thank you for dealing with flaky tests - I appreciate that >>> effort very much, and the goal of my comments is just to further >>> increase the quality of the tests. >>> >>> >>> http://codereview.chromium.**org/7104081/<http://codereview.chromium.org/7104... >>> >> >> > |
