Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(468)

Issue 9460019: Reduce flakiness in chrome_frame_tests.exe by having each run in a clean environment. (Closed)

Created:
8 years, 10 months ago by grt (UTC plus 2)
Modified:
8 years, 9 months ago
Reviewers:
robertshield
CC:
chromium-reviews, amit
Visibility:
Public.

Description

Reduce flakiness in chrome_frame_tests.exe by having each test run in a clean environment. This includes: * A TestScrubber runs between all test to kill stray IE and Chrome processes and delete the user data dir. * Refactored CFACWithChrome and ProxyFactoryTest tests to get rid of copy-n-paste. * Tests in ChromeFrameTestWithWebServer that launch Chrome now use a fresh user data dir so as not to collide with a users' existing profile. BUG=81479, 114386 TEST=chrome_frame_tests.exe is green on the win_cf trybot Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=123973

Patch Set 1 #

Patch Set 2 : fixed compile break #

Total comments: 19

Patch Set 3 : addressed robert's comments #

Total comments: 8

Patch Set 4 : addressed final comments #

Patch Set 5 : fixed compile break #

Unified diffs Side-by-side diffs Delta from patch set Stats (+418 lines, -250 lines) Patch
M chrome_frame/chrome_frame.gyp View 2 chunks +4 lines, -0 lines 0 comments Download
M chrome_frame/test/automation_client_mock.cc View 1 2 3 3 chunks +94 lines, -103 lines 0 comments Download
M chrome_frame/test/chrome_frame_automation_mock.h View 3 chunks +3 lines, -1 line 0 comments Download
M chrome_frame/test/chrome_frame_automation_mock.cc View 2 chunks +2 lines, -8 lines 0 comments Download
M chrome_frame/test/chrome_frame_test_utils.h View 2 chunks +3 lines, -1 line 0 comments Download
M chrome_frame/test/chrome_frame_test_utils.cc View 1 2 3 4 1 chunk +4 lines, -1 line 0 comments Download
M chrome_frame/test/proxy_factory_mock.cc View 3 chunks +59 lines, -60 lines 0 comments Download
M chrome_frame/test/run_all_unittests.cc View 2 chunks +3 lines, -0 lines 0 comments Download
A chrome_frame/test/test_scrubber.h View 1 2 3 1 chunk +36 lines, -0 lines 0 comments Download
A chrome_frame/test/test_scrubber.cc View 1 2 3 4 1 chunk +146 lines, -0 lines 0 comments Download
M chrome_frame/test/test_with_web_server.h View 5 chunks +19 lines, -27 lines 0 comments Download
M chrome_frame/test/test_with_web_server.cc View 1 2 6 chunks +42 lines, -36 lines 0 comments Download
M chrome_frame/test/ui_test.cc View 1 chunk +1 line, -2 lines 0 comments Download
M chrome_frame/test_utils.cc View 2 chunks +2 lines, -11 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
grt (UTC plus 2)
8 years, 10 months ago (2012-02-24 21:22:54 UTC) #1
grt (UTC plus 2)
The first try run will fail since I forgot to force the job to sync ...
8 years, 10 months ago (2012-02-24 21:28:15 UTC) #2
robertshield
looking good, nits below http://codereview.chromium.org/9460019/diff/4001/chrome_frame/test/automation_client_mock.cc File chrome_frame/test/automation_client_mock.cc (right): http://codereview.chromium.org/9460019/diff/4001/chrome_frame/test/automation_client_mock.cc#newcode103 chrome_frame/test/automation_client_mock.cc:103: class CFACWithChrome : public testing::Test ...
8 years, 10 months ago (2012-02-27 21:13:42 UTC) #3
grt (UTC plus 2)
Thanks! PTAL. http://codereview.chromium.org/9460019/diff/4001/chrome_frame/test/automation_client_mock.cc File chrome_frame/test/automation_client_mock.cc (right): http://codereview.chromium.org/9460019/diff/4001/chrome_frame/test/automation_client_mock.cc#newcode103 chrome_frame/test/automation_client_mock.cc:103: class CFACWithChrome : public testing::Test { On ...
8 years, 9 months ago (2012-02-28 03:12:04 UTC) #4
robertshield
lgtm, a few minor nits. http://codereview.chromium.org/9460019/diff/4001/chrome_frame/test/test_scrubber.cc File chrome_frame/test/test_scrubber.cc (right): http://codereview.chromium.org/9460019/diff/4001/chrome_frame/test/test_scrubber.cc#newcode30 chrome_frame/test/test_scrubber.cc:30: namespace { On 2012/02/28 ...
8 years, 9 months ago (2012-02-28 03:32:15 UTC) #5
grt (UTC plus 2)
8 years, 9 months ago (2012-02-28 04:03:24 UTC) #6
Groovy, thanks for the review.  I'll dcommit in the morning so I can monitor the
fallout on the main waterfall.

http://codereview.chromium.org/9460019/diff/9001/chrome_frame/test/automation...
File chrome_frame/test/automation_client_mock.cc (right):

http://codereview.chromium.org/9460019/diff/9001/chrome_frame/test/automation...
chrome_frame/test/automation_client_mock.cc:106: const int
kSaneAutomationTimeoutMs = 10 * 1000;
On 2012/02/28 03:32:15, robertshield wrote:
> Would you consider moving this up to the timeouts at the top of the file? It's
> nice to have all the timeout values in the same place IMO.

Done.

http://codereview.chromium.org/9460019/diff/9001/chrome_frame/test/automation...
chrome_frame/test/automation_client_mock.cc:242: int timeout = 500;
On 2012/02/28 03:32:15, robertshield wrote:
> since we're here, could you constantize this one too?

I'd rather not.  I'm not sure why 500ms was chosen for the timeout, so I don't
have a sensible name for the constant (other than kOneHalfSecondMs).  I didn't
touch these tests in the CL, and I don't really know exactly how they differ
from the CFACWithChrome tests.

http://codereview.chromium.org/9460019/diff/9001/chrome_frame/test/automation...
chrome_frame/test/automation_client_mock.cc:419: int timeout = 500;
On 2012/02/28 03:32:15, robertshield wrote:
> also here

Ditto.

http://codereview.chromium.org/9460019/diff/9001/chrome_frame/test/test_scrub...
File chrome_frame/test/test_scrubber.h (right):

http://codereview.chromium.org/9460019/diff/9001/chrome_frame/test/test_scrub...
chrome_frame/test/test_scrubber.h:14: // automation_client_mock.cc for an
example.
On 2012/02/28 03:32:15, robertshield wrote:
> The last sentence is prone to rapid bitrotting (likely to get invalidated if
> anyone comes in and tries to refactor this stuff).  I'd remove it (or at least
> remove the file name) since the curious can search for usages using
technology.

Removed.  I initially changed it to "search for it to see good examples," but
since that's almost universally true of all code (except where it isn't), it
didn't seem to add value.

Powered by Google App Engine
This is Rietveld 408576698