Fix up various browser tests not to use WaitForNavigation.

Fix up various browser tests not to use WaitForNavigation.
Convenience methods for WindowedNotificationObserver.

R=phajdan.jr@chromium.org
BUG=various
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96593

[Paweł Hajdan Jr., 2011-07-25 18:06:13]

Thank you very much for working on this. Generally minor comments. Looking
forward to the removal of inherently flaky calls.

http://codereview.chromium.org/7489019/diff/1/chrome/browser/ssl/ssl_browser_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/7489019/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:121: // Wait for the navigation to be
done.
nit: Nuke comments like this, I think it's redundant.

http://codereview.chromium.org/7489019/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:1045: EXPECT_TRUE(success);
I think this should be an ASSERT to avoid hangs. Applies to entire CL.

http://codereview.chromium.org/7489019/diff/1/chrome/test/ui_test_utils.h
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/7489019/diff/1/chrome/test/ui_test_utils.h#new...
chrome/test/ui_test_utils.h:419: explicit WindowedNotificationObserver(const
Browser& browser);
Please don't add those "magic constructors" here, but rather in a new class
(like WindowedLoadStopObserver/WindowedNavigationObserver).

[Greg Billock, 2011-08-02 18:11:33]

http://codereview.chromium.org/7489019/diff/1/chrome/browser/ssl/ssl_browser_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/7489019/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:121: // Wait for the navigation to be
done.
On 2011/07/25 18:06:13, Paweł Hajdan Jr. wrote:
> nit: Nuke comments like this, I think it's redundant.

Done.

http://codereview.chromium.org/7489019/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:1045: EXPECT_TRUE(success);
On 2011/07/25 18:06:13, Paweł Hajdan Jr. wrote:
> I think this should be an ASSERT to avoid hangs. Applies to entire CL.

OK, I think I got them all.

http://codereview.chromium.org/7489019/diff/1/chrome/test/ui_test_utils.h
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/7489019/diff/1/chrome/test/ui_test_utils.h#new...
chrome/test/ui_test_utils.h:419: explicit WindowedNotificationObserver(const
Browser& browser);
On 2011/07/25 18:06:13, Paweł Hajdan Jr. wrote:
> Please don't add those "magic constructors" here, but rather in a new class
> (like WindowedLoadStopObserver/WindowedNavigationObserver).

I can do that. Shall I make two new subclasses here? My inclination for putting
them here is that they're basically just alternate forms for the
NotificationSource, which can be kind of verbose and template-y.

I'm suspicious they'll be more discoverable in this one notification observer
class than separated out.

[Paweł Hajdan Jr., 2011-08-02 20:11:19]

If possible, it would be nice to do something with the TestNavigationObserver
duplication/divergence. I'm not really sure how important those differences are,
but the duplication is bad. Anyway, if that'd delay this CL please do that in a
separate one.

I think that getting completely rid of the racy functions should be the
priority, then we can deduplicate.

http://codereview.chromium.org/7489019/diff/1/chrome/test/ui_test_utils.h
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/7489019/diff/1/chrome/test/ui_test_utils.h#new...
chrome/test/ui_test_utils.h:419: explicit WindowedNotificationObserver(const
Browser& browser);
On 2011/08/02 18:11:33, Greg Billock wrote:
> On 2011/07/25 18:06:13, Paweł Hajdan Jr. wrote:
> > Please don't add those "magic constructors" here, but rather in a new class
> > (like WindowedLoadStopObserver/WindowedNavigationObserver).
> 
> I can do that. Shall I make two new subclasses here? My inclination for
putting
> them here is that they're basically just alternate forms for the
> NotificationSource, which can be kind of verbose and template-y.
> 
> I'm suspicious they'll be more discoverable in this one notification observer
> class than separated out.

Well, do we actually need those ctors? Why not do what they do in the calling
code?

[Greg Billock, 2011-08-03 18:08:29]

On 2011/08/02 20:11:19, Paweł Hajdan Jr. wrote:
> If possible, it would be nice to do something with the TestNavigationObserver
> duplication/divergence. I'm not really sure how important those differences
are,
> but the duplication is bad. Anyway, if that'd delay this CL please do that in
a
> separate one.
> 
> I think that getting completely rid of the racy functions should be the
> priority, then we can deduplicate.

I think that makes sense. Marked this with a TODO for now.
 
 http://codereview.chromium.org/7489019/diff/1/chrome/test/ui_test_utils.h
> File chrome/test/ui_test_utils.h (right):
> 
>
http://codereview.chromium.org/7489019/diff/1/chrome/test/ui_test_utils.h#new...
> chrome/test/ui_test_utils.h:419: explicit WindowedNotificationObserver(const
> Browser& browser);
> On 2011/08/02 18:11:33, Greg Billock wrote:
> > On 2011/07/25 18:06:13, Paweł Hajdan Jr. wrote:
> > > Please don't add those "magic constructors" here, but rather in a new
class
> > > (like WindowedLoadStopObserver/WindowedNavigationObserver).
> > 
> > I can do that. Shall I make two new subclasses here? My inclination for
> putting
> > them here is that they're basically just alternate forms for the
> > NotificationSource, which can be kind of verbose and template-y.
> > 
> > I'm suspicious they'll be more discoverable in this one notification
observer
> > class than separated out.
> 
> Well, do we actually need those ctors? Why not do what they do in the calling
> code?

Basically they're conveniences. It's pretty wordy to make a NavigationSource,
and lends almost nothing to the test readability where they occur. This usage is
pretty common for the kind of idiom I'm replacing, so I made these convenience
constructors. I have a mild preference to keep them here for discoverability,
but if you think they're better put in subclasses, I can move them.

[Paweł Hajdan Jr., 2011-08-03 19:13:09]

On 2011/08/03 18:08:29, Greg Billock wrote:
> Basically they're conveniences. It's pretty wordy to make a NavigationSource,
> and lends almost nothing to the test readability where they occur. This usage
is
> pretty common for the kind of idiom I'm replacing, so I made these convenience
> constructors. I have a mild preference to keep them here for discoverability,
> but if you think they're better put in subclasses, I can move them.

I still don't get it why don't we just do it in the caller. It is a bit wordy,
but so is every other usage of WindowedNotificationObserver. Explicit is better
than implicit, and I don't really like the hidden notification type in this
case.

[Greg Billock, 2011-08-03 23:08:12]

On 2011/08/03 19:13:09, Paweł Hajdan Jr. wrote:
> On 2011/08/03 18:08:29, Greg Billock wrote:
> > Basically they're conveniences. It's pretty wordy to make a
NavigationSource,
> > and lends almost nothing to the test readability where they occur. This
usage
> is
> > pretty common for the kind of idiom I'm replacing, so I made these
convenience
> > constructors. I have a mild preference to keep them here for
discoverability,
> > but if you think they're better put in subclasses, I can move them.
> 
> I still don't get it why don't we just do it in the caller. It is a bit wordy,
> but so is every other usage of WindowedNotificationObserver. Explicit is
better
> than implicit, and I don't really like the hidden notification type in this
> case.

OK. I moved them.

[Paweł Hajdan Jr., 2011-08-04 17:56:48]

LGTM with comments, thanks a lot for this work!

http://codereview.chromium.org/7489019/diff/15001/chrome/browser/sidebar/side...
File chrome/browser/sidebar/sidebar_browsertest.cc (right):

http://codereview.chromium.org/7489019/diff/15001/chrome/browser/sidebar/side...
chrome/browser/sidebar/sidebar_browsertest.cc:75:
ui_test_utils::WindowedNotificationObserver observer(tab->controller());
Looks like you may need to update this call site to use the existing ctor.

http://codereview.chromium.org/7489019/diff/15001/chrome/browser/sidebar/side...
chrome/browser/sidebar/sidebar_browsertest.cc:79: SidebarContainer*
sidebar_container =
nit: Looks like this could be removed now, below too.

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.cc (right):

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.cc:60: // TODO(gbillock): this should be merged
with TestNavigationObserver
Thanks, and IMHO this is a very high priority. If you have cycles to do that
right after this change, I'd very much appreciate that.

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.h (right):

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.h:411: 
nit: I'd prefer not to add an empty line here.

[Greg Billock, 2011-08-11 22:57:45]

http://codereview.chromium.org/7489019/diff/15001/chrome/browser/sidebar/side...
File chrome/browser/sidebar/sidebar_browsertest.cc (right):

http://codereview.chromium.org/7489019/diff/15001/chrome/browser/sidebar/side...
chrome/browser/sidebar/sidebar_browsertest.cc:75:
ui_test_utils::WindowedNotificationObserver observer(tab->controller());
On 2011/08/04 17:56:48, Paweł Hajdan Jr. wrote:
> Looks like you may need to update this call site to use the existing ctor.

Yeah, a try caught that, too. It's changed and uploaded.

http://codereview.chromium.org/7489019/diff/15001/chrome/browser/sidebar/side...
chrome/browser/sidebar/sidebar_browsertest.cc:79: SidebarContainer*
sidebar_container =
On 2011/08/04 17:56:48, Paweł Hajdan Jr. wrote:
> nit: Looks like this could be removed now, below too.

Done.

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.cc (right):

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.cc:60: // TODO(gbillock): this should be merged
with TestNavigationObserver
On 2011/08/04 17:56:48, Paweł Hajdan Jr. wrote:
> Thanks, and IMHO this is a very high priority. If you have cycles to do that
> right after this change, I'd very much appreciate that.

Sounds good. I'm planning on a sequence of these as I have some time, and that's
next on the list. Hopefully it'll include some FLAKY unmarking! :-) Is there a
utility we have for running lots of iterations of a test a la blaze test
--runs_per_test?

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.cc:60: // TODO(gbillock): this should be merged
with TestNavigationObserver
On 2011/08/04 17:56:48, Paweł Hajdan Jr. wrote:
> Thanks, and IMHO this is a very high priority. If you have cycles to do that
> right after this change, I'd very much appreciate that.

Yeah, I plan on a series of changes, and this is next on the list. Hopefully
some will be to unmark FLAKY. :-) Do we have an analog to blaze test
runs_per_test?

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.h (right):

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.h:411: 
On 2011/08/04 17:56:48, Paweł Hajdan Jr. wrote:
> nit: I'd prefer not to add an empty line here.

Done.

[Paweł Hajdan Jr., 2011-08-11 23:03:02]

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
File chrome/test/base/ui_test_utils.cc (right):

http://codereview.chromium.org/7489019/diff/15001/chrome/test/base/ui_test_ut...
chrome/test/base/ui_test_utils.cc:60: // TODO(gbillock): this should be merged
with TestNavigationObserver
On 2011/08/11 22:57:45, Greg Billock wrote:
> On 2011/08/04 17:56:48, Paweł Hajdan Jr. wrote:
> > Thanks, and IMHO this is a very high priority. If you have cycles to do that
> > right after this change, I'd very much appreciate that.
> 
> Yeah, I plan on a series of changes, and this is next on the list. Hopefully
> some will be to unmark FLAKY. :-) Do we have an analog to blaze test
> runs_per_test?

You can try --gtest_repeat=-1 --gtest_break_on_failure, but of course that can't
_prove_ a test is not flaky (but can prove that it is).

Anyway, let's focus on removing risky functions primarily. If that allows us to
remove some FLAKY marks, that's great, but isn't my primary concern.

[commit-bot: I haz the power, 2011-08-12 19:17:52]

Change committed as 96593