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

Issue 9414024: Fix flaky SSLUI tests (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 2 months ago by tpayne
Modified:
2 years, 2 months ago
Reviewers:
agl, wtc, Ryan Sleevi, dpapad
CC:
chromium-reviews_chromium.org, vandebo
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Fix flaky SSLUI tests.

These tests seem to be occasionally having extraneous SSL errors. I changed the tests to check that the expected errors occur and log a warning about the extraneous errors.


BUG=40932, 43575
TEST=


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

Patch Set 1 #

Total comments: 10

Patch Set 2 : Addressed Comments #

Total comments: 2

Patch Set 3 : Addressed Comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -27 lines) Lint Patch
M chrome/browser/ssl/ssl_browser_tests.cc View 1 2 12 chunks +17 lines, -27 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 12
tpayne
2 years, 2 months ago #1
agl
Do you have an example of it failing (i.e. the failing values)?
2 years, 2 months ago #2
tpayne
On 2012/02/16 22:31:32, agl wrote: > Do you have an example of it failing (i.e. ...
2 years, 2 months ago #3
dpapad
http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_tests.cc#newcode368 chrome/browser/ssl/ssl_browser_tests.cc:368: // See bug 114185. Could you also remove this ...
2 years, 2 months ago #4
wtc
Patch Set 1 LGTM. Just some nits. http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_tests.cc#newcode103 chrome/browser/ssl/ssl_browser_tests.cc:103: entry->GetSSL().cert_status & ...
2 years, 2 months ago #5
tpayne
http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_tests.cc#newcode103 chrome/browser/ssl/ssl_browser_tests.cc:103: entry->GetSSL().cert_status & net::CERT_STATUS_ALL_ERRORS); On 2012/02/17 00:14:07, wtc wrote: > ...
2 years, 2 months ago #6
wtc
Patch Set 2 LGTM. One nit. http://codereview.chromium.org/9414024/diff/7001/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): http://codereview.chromium.org/9414024/diff/7001/chrome/browser/ssl/ssl_browser_tests.cc#newcode401 chrome/browser/ssl/ssl_browser_tests.cc:401: // See bug ...
2 years, 2 months ago #7
tpayne
http://codereview.chromium.org/9414024/diff/7001/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): http://codereview.chromium.org/9414024/diff/7001/chrome/browser/ssl/ssl_browser_tests.cc#newcode401 chrome/browser/ssl/ssl_browser_tests.cc:401: // See bug 114185. On 2012/02/17 19:43:03, wtc wrote: ...
2 years, 2 months ago #8
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tpayne@chromium.org/9414024/10001
2 years, 2 months ago #9
I haz the power (commit-bot)
Change committed as 122600
2 years, 2 months ago #10
Ryan Sleevi
Prior to joining Google, I had spent quite a bit of time looking into this. ...
2 years, 2 months ago #11
tpayne
2 years, 2 months ago #12
On 2012/02/17 22:28:45, Ryan Sleevi wrote:
> Prior to joining Google, I had spent quite a bit of time looking into this.
> 
> The DISABLED_ tests were indeed failing because of a variety of reasons - the
> test server failing to start, workers hanging the process, and, more commonly,
> /NO/ security error being reported.
> 
> The lack of a security error apparently has to do with raciness between the
test
> automation and the time the browser takes to navigate/load. Additionally, some
> layer (somewhere) would do redirects to about:blank, rather than an
> interstitial.
> 
> I'd love to think this fixes them, but please keep an eye on the waterfall and
> the build logs for the next few weeks to ensure this doesn't regress anything.
> 
> http://chromium-build-logs.appspot.com/ is a good place to go to get archives
of
> the build logs, but you'll also want to watch the flakiness dashboard.

Thanks, Ryan. I, too, am not 100% convinced these are completely fixed. I'll be
keeping a close eye on this.
Sign in to reply to this message.

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