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

Issue 9414024: Fix flaky SSLUI tests (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 1 month ago by tpayne
Modified:
3 years, 1 month ago
Reviewers:
agl, wtc, Ryan Sleevi, dpapad
CC:
chromium-reviews, vandebo (ex-Chrome)
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) Patch
M chrome/browser/ssl/ssl_browser_tests.cc View 1 2 12 chunks +17 lines, -27 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 12 (0 generated)
tpayne
3 years, 1 month ago (2012-02-16 22:28:01 UTC) #1
agl
Do you have an example of it failing (i.e. the failing values)?
3 years, 1 month ago (2012-02-16 22:31:32 UTC) #2
tpayne
On 2012/02/16 22:31:32, agl wrote: > Do you have an example of it failing (i.e. ...
3 years, 1 month ago (2012-02-16 22:33:46 UTC) #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 ...
3 years, 1 month ago (2012-02-16 23:48:22 UTC) #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 & ...
3 years, 1 month ago (2012-02-17 00:14:06 UTC) #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: > ...
3 years, 1 month ago (2012-02-17 19:25:47 UTC) #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 ...
3 years, 1 month ago (2012-02-17 19:43:03 UTC) #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: ...
3 years, 1 month ago (2012-02-17 19:47:38 UTC) #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
3 years, 1 month ago (2012-02-17 20:51:43 UTC) #9
I haz the power (commit-bot)
Change committed as 122600
3 years, 1 month ago (2012-02-17 22:10:32 UTC) #10
Ryan Sleevi
Prior to joining Google, I had spent quite a bit of time looking into this. ...
3 years, 1 month ago (2012-02-17 22:28:45 UTC) #11
tpayne
3 years, 1 month ago (2012-02-17 22:30:13 UTC) #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 cf4c24d