Chromium Code Reviews
Help | Chromium Project | Sign in
(891)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by grt
Modified:
2 years, 1 month ago
Reviewers:
robertshield
CC:
chromium-reviews_chromium.org, 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) Lint Patch
M chrome_frame/chrome_frame.gyp View 2 chunks +4 lines, -0 lines 0 comments ? errors Download
M chrome_frame/test/automation_client_mock.cc View 1 2 3 3 chunks +94 lines, -103 lines 0 comments 0 errors Download
M chrome_frame/test/chrome_frame_automation_mock.h View 3 chunks +3 lines, -1 line 0 comments 0 errors Download
M chrome_frame/test/chrome_frame_automation_mock.cc View 2 chunks +2 lines, -8 lines 0 comments 0 errors Download
M chrome_frame/test/chrome_frame_test_utils.h View 2 chunks +3 lines, -1 line 0 comments 0 errors Download
M chrome_frame/test/chrome_frame_test_utils.cc View 1 2 3 4 1 chunk +4 lines, -1 line 0 comments ? errors Download
M chrome_frame/test/proxy_factory_mock.cc View 3 chunks +59 lines, -60 lines 0 comments 0 errors Download
M chrome_frame/test/run_all_unittests.cc View 2 chunks +3 lines, -0 lines 0 comments 0 errors Download
A chrome_frame/test/test_scrubber.h View 1 2 3 1 chunk +36 lines, -0 lines 0 comments 1 errors Download
A chrome_frame/test/test_scrubber.cc View 1 2 3 4 1 chunk +146 lines, -0 lines 0 comments 1 errors Download
M chrome_frame/test/test_with_web_server.h View 5 chunks +19 lines, -27 lines 0 comments 0 errors Download
M chrome_frame/test/test_with_web_server.cc View 1 2 6 chunks +42 lines, -36 lines 0 comments 1 errors Download
M chrome_frame/test/ui_test.cc View 1 chunk +1 line, -2 lines 0 comments 0 errors Download
M chrome_frame/test_utils.cc View 2 chunks +2 lines, -11 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 6
grt
2 years, 1 month ago #1
grt
The first try run will fail since I forgot to force the job to sync ...
2 years, 1 month ago #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 ...
2 years, 1 month ago #3
grt
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 ...
2 years, 1 month ago #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 ...
2 years, 1 month ago #5
grt
2 years, 1 month ago #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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6