Attempt to fix ChromeFrameTestWithWebServer tests.

Attempt to fix ChromeFrameTestWithWebServer tests.

- Use a single MockWebServer (HTTPTestServer) instance for the whole test case rather than one per instance.
- Run pending tasks and the end of each test.
- Try even harder to keep the browser from caching.
- Re-enable all previously disabled tests.
- Save a snapshot of the screen to the desktop (ChromiumSnapshotYYYYMMDDHHMMSS.png) when tests timeout.
- Use a local IPv4 address assigned to a NIC rather than the loopback address.
- Retry tests that timeout a single time.

BUG=112599, 96449, 37088, 32321, 111074
TEST=chrome_frame_tests.exe --gtest_filter=ChromeFrameTestWithWebServer.* becomes less flaky

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

[grt (UTC plus 2), 2012-04-06 18:44:21]

I wonder if this will make any difference whatsoever...

[robertshield, 2012-04-06 21:16:14]

Change LGTM, see comment below

https://chromiumcodereview.appspot.com/10007043/diff/1/chrome_frame/test/test...
File chrome_frame/test/test_with_web_server.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/1/chrome_frame/test/test...
chrome_frame/test/test_with_web_server.cc:115: 1337, L"127.0.0.1",
chrome_frame_test::GetTestDataFolder());
I wonder if Erik's code to listen on one of the actual network interfaces would
be useful here?

[grt (UTC plus 2), 2012-04-06 23:00:33]

I'll give it a shot, as this doesn't seem to have helped.
On Apr 6, 2012 5:16 PM, <robertshield@chromium.org> wrote:

> Change LGTM, see comment below
>
>
> https://chromiumcodereview.**appspot.com/10007043/diff/1/**
>
chrome_frame/test/test_with_**web_server.cc<https://chromiumcodereview.appspot.com/10007043/diff/1/chrome_frame/test/test_with_web_server.cc>
> File chrome_frame/test/test_with_**web_server.cc (right):
>
> https://chromiumcodereview.**appspot.com/10007043/diff/1/**
>
chrome_frame/test/test_with_**web_server.cc#newcode115<https://chromiumcodereview.appspot.com/10007043/diff/1/chrome_frame/test/test_with_web_server.cc#newcode115>
> chrome_frame/test/test_with_**web_server.cc:115: 1337, L"127.0.0.1",
> chrome_frame_test::**GetTestDataFolder());
> I wonder if Erik's code to listen on one of the actual network
> interfaces would be useful here?
>
>
https://chromiumcodereview.**appspot.com/10007043/<https://chromiumcodereview...
>

[grt (UTC plus 2), 2012-04-09 14:02:39]

https://chromiumcodereview.appspot.com/10007043/diff/1/chrome_frame/test/test...
File chrome_frame/test/test_with_web_server.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/1/chrome_frame/test/test...
chrome_frame/test/test_with_web_server.cc:115: 1337, L"127.0.0.1",
chrome_frame_test::GetTestDataFolder());
On 2012/04/06 21:16:14, robertshield wrote:
> I wonder if Erik's code to listen on one of the actual network interfaces
would
> be useful here?

Good suggestion.  I tried it out and it didn't help.  Same thing as before. 
Check out the log for the latest win_rel failure.

I'm going to add code to retry once before failing the test unless you have a
better idea (e.g., mark the whole suite as flaky).

https://chromiumcodereview.appspot.com/10007043/diff/9001/chrome/test/base/ui...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/9001/chrome/test/base/ui...
chrome/test/base/ui_test_utils.cc:11: #include <algorithm>
remove these

[grt (UTC plus 2), 2012-04-11 03:23:30]

This CL currently contains the TimeDelta change in
https://chromiumcodereview.appspot.com/10053004/.  I'll remove it from here once
that one lands.

I think the retry is the only thing about this CL that may lead to greener runs.
 Let me know if you'd like to back out any of the other changes.

[robertshield, 2012-04-11 17:03:36]

LGTM, with the obligatory nits, moaning and complaining to keep you happy.

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/browser/ui/...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/browser/ui/...
chrome/browser/ui/window_snapshot/window_snapshot.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/browser/ui/...
File chrome/browser/ui/window_snapshot/window_snapshot_win.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/browser/ui/...
chrome/browser/ui/window_snapshot/window_snapshot_win.cc:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/browser/ui/...
chrome/browser/ui/window_snapshot/window_snapshot_win.cc:19: if (window_handle)
{
perhaps if (IsWindow(window_handle))?

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:1200: out_path,
reinterpret_cast<char*>(&png_data[0]), bytes);
should we also first check that png_data is not empty?

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
File chrome_frame/test/automation_client_mock.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
chrome_frame/test/automation_client_mock.cc:337: const base::TimeDelta
loop_duration = base::TimeDelta::FromSeconds(11);
loop_duration -> loop_duration_seconds

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
File chrome_frame/test/chrome_frame_test_utils.h (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
chrome_frame/test/chrome_frame_test_utils.h:225: QuitAfter(base::TimeDelta());
nit: add a comment to indicate that this means "quit now".

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
File chrome_frame/test/mock_ie_event_sink_test.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
chrome_frame/test/mock_ie_event_sink_test.cc:205: hung_call_detector_ =
HungCOMCallDetector::Setup(ceil(timeout.InSecondsF()));
why no InSeconds()?

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
File chrome_frame/test/test_with_web_server.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
chrome_frame/test/test_with_web_server.cc:255: ASSERT_EQ(0, tries++) << "Test
timed out twice.";
Please explicitly mark this with a comment as the expected exit point from the
loop when we time out twice.

[grt (UTC plus 2), 2012-04-11 17:33:08]

I'm so happy.  Thanks.  PTAL.

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/browser/ui/...
File chrome/browser/ui/window_snapshot/window_snapshot_win.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/browser/ui/...
chrome/browser/ui/window_snapshot/window_snapshot_win.cc:19: if (window_handle)
{
On 2012/04/11 17:03:36, robertshield wrote:
> perhaps if (IsWindow(window_handle))?

I don't think so.  NULL specifically means the desktop.  Other values that
aren't windows shouldn't be treated as such.

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:1200: out_path,
reinterpret_cast<char*>(&png_data[0]), bytes);
On 2012/04/11 17:03:36, robertshield wrote:
> should we also first check that png_data is not empty?

I don't see why.  GrabWindowSnapshot's contract is such that it returns false if
it doesn't produce data.

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
File chrome_frame/test/automation_client_mock.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
chrome_frame/test/automation_client_mock.cc:337: const base::TimeDelta
loop_duration = base::TimeDelta::FromSeconds(11);
On 2012/04/11 17:03:36, robertshield wrote:
> loop_duration -> loop_duration_seconds

I disagree.  That implies that |loop_duration|'s units are seconds, which is not
the case.

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
File chrome_frame/test/chrome_frame_test_utils.h (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
chrome_frame/test/chrome_frame_test_utils.h:225: QuitAfter(base::TimeDelta());
On 2012/04/11 17:03:36, robertshield wrote:
> nit: add a comment to indicate that this means "quit now".

Done.

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
File chrome_frame/test/mock_ie_event_sink_test.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
chrome_frame/test/mock_ie_event_sink_test.cc:205: hung_call_detector_ =
HungCOMCallDetector::Setup(ceil(timeout.InSecondsF()));
On 2012/04/11 17:03:36, robertshield wrote:
> why no InSeconds()?

InSeconds would return 0 if timeout is less than 500ms, which will likely be too
short for the detector.

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
File chrome_frame/test/test_with_web_server.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
chrome_frame/test/test_with_web_server.cc:255: ASSERT_EQ(0, tries++) << "Test
timed out twice.";
On 2012/04/11 17:03:36, robertshield wrote:
> Please explicitly mark this with a comment as the expected exit point from the
> loop when we time out twice.

Done.

[robertshield, 2012-04-11 17:46:07]

On 2012/04/11 17:33:08, grt wrote:
> I'm so happy.  Thanks.  PTAL.
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/browser/ui/...
> File chrome/browser/ui/window_snapshot/window_snapshot_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/browser/ui/...
> chrome/browser/ui/window_snapshot/window_snapshot_win.cc:19: if
(window_handle)
> {
> On 2012/04/11 17:03:36, robertshield wrote:
> > perhaps if (IsWindow(window_handle))?
> 
> I don't think so.  NULL specifically means the desktop.  Other values that
> aren't windows shouldn't be treated as such.
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/test/base/u...
> File chrome/test/base/ui_test_utils.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome/test/base/u...
> chrome/test/base/ui_test_utils.cc:1200: out_path,
> reinterpret_cast<char*>(&png_data[0]), bytes);
> On 2012/04/11 17:03:36, robertshield wrote:
> > should we also first check that png_data is not empty?
> 
> I don't see why.  GrabWindowSnapshot's contract is such that it returns false
if
> it doesn't produce data.
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
> File chrome_frame/test/automation_client_mock.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
> chrome_frame/test/automation_client_mock.cc:337: const base::TimeDelta
> loop_duration = base::TimeDelta::FromSeconds(11);
> On 2012/04/11 17:03:36, robertshield wrote:
> > loop_duration -> loop_duration_seconds
> 
> I disagree.  That implies that |loop_duration|'s units are seconds, which is
not
> the case.
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
> File chrome_frame/test/chrome_frame_test_utils.h (right):
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
> chrome_frame/test/chrome_frame_test_utils.h:225: QuitAfter(base::TimeDelta());
> On 2012/04/11 17:03:36, robertshield wrote:
> > nit: add a comment to indicate that this means "quit now".
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
> File chrome_frame/test/mock_ie_event_sink_test.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
> chrome_frame/test/mock_ie_event_sink_test.cc:205: hung_call_detector_ =
> HungCOMCallDetector::Setup(ceil(timeout.InSecondsF()));
> On 2012/04/11 17:03:36, robertshield wrote:
> > why no InSeconds()?
> 
> InSeconds would return 0 if timeout is less than 500ms, which will likely be
too
> short for the detector.
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
> File chrome_frame/test/test_with_web_server.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10007043/diff/25005/chrome_frame/test/...
> chrome_frame/test/test_with_web_server.cc:255: ASSERT_EQ(0, tries++) << "Test
> timed out twice.";
> On 2012/04/11 17:03:36, robertshield wrote:
> > Please explicitly mark this with a comment as the expected exit point from
the
> > loop when we time out twice.
> 
> Done.

LGTM++

[grt (UTC plus 2), 2012-04-11 20:21:59]

OWNERS time.  please let me know if someone else is better suited to review
these changes.

ben: chrome/browser/ui
pawel: chrome/test

thanks.

[Ben Goodger (Google), 2012-04-11 21:10:26]

LGTM for browser/ui

[grt (UTC plus 2), 2012-04-13 15:25:03]

Pawel: ping.

[Paweł Hajdan Jr., 2012-04-14 10:42:56]

Sorry for late response. How do you prevent those on-disk snapshots from filling
up disk space? This is a _must_ have, we recently had a problem with other
temporary files filling up disks.

https://chromiumcodereview.appspot.com/10007043/diff/25009/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25009/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:279: #if defined(OS_WIN)
Please extract this and any other possible Windows-specific functions to
ui_test_utils_win.cc

[grt (UTC plus 2), 2012-04-14 23:18:04]

Thanks, Pawel.  See https://chromiumcodereview.appspot.com/9972002/ for The
Disks Are Too Damn Full.

https://chromiumcodereview.appspot.com/10007043/diff/25009/chrome/test/base/u...
File chrome/test/base/ui_test_utils.cc (right):

https://chromiumcodereview.appspot.com/10007043/diff/25009/chrome/test/base/u...
chrome/test/base/ui_test_utils.cc:279: #if defined(OS_WIN)
On 2012/04/14 10:42:56, Paweł Hajdan Jr. wrote:
> Please extract this and any other possible Windows-specific functions to
> ui_test_utils_win.cc

Done.

[Paweł Hajdan Jr., 2012-04-17 06:27:54]

LGTM

[commit-bot: I haz the power, 2012-04-17 17:35:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10007043/32001

[commit-bot: I haz the power, 2012-04-17 18:53:48]

Try job failure for 10007043-32001 on linux_clang for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-04-17 18:58:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10007043/32001

[commit-bot: I haz the power, 2012-04-17 20:34:34]

Change committed as 132637