Fix flaky SSLUI tests

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

[agl, 2012-02-16 22:31:32]

Do you have an example of it failing (i.e. the failing values)?

[tpayne, 2012-02-16 22:33:46]

On 2012/02/16 22:31:32, agl wrote:
> Do you have an example of it failing (i.e. the failing values)?

When it fails, I have only seen the extraneous value be 2
(CERT_STATUS_DATE_INVALID)

[dpapad, 2012-02-16 23:48:22]

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:368: // See bug 114185.
Could you also remove this comment and add the bug number to the cl description?

[wtc, 2012-02-17 00:14:06]

Patch Set 1 LGTM.  Just some nits.

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:103: entry->GetSSL().cert_status &
net::CERT_STATUS_ALL_ERRORS);

If you want to allow extraneous certificate errors, you can
just say:
  EXPECT_EQ(error, entry->GetSSL().cert_status & error);

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:108: unsigned int extra_cert_errors =
error ^ (entry->GetSSL().cert_status &

Nit: it looks nicer to use net::CertStatus instead of unsigned int.

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:112: }

Nit: this file seems to omit curly braces for one-liner if
statements, so we should follow that style.

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:368: // See bug 114185.

On 2012/02/16 23:48:22, dpapad wrote:
> Could you also remove this comment and add the bug number to the cl
description?

I believe we need to keep the "See bug 114185." comment
because this unit test is a regression test for that bug.

[tpayne, 2012-02-17 19:25:47]

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
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:
> 
> If you want to allow extraneous certificate errors, you can
> just say:
>   EXPECT_EQ(error, entry->GetSSL().cert_status & error);

Done.

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:108: unsigned int extra_cert_errors =
error ^ (entry->GetSSL().cert_status &
On 2012/02/17 00:14:07, wtc wrote:
> 
> Nit: it looks nicer to use net::CertStatus instead of unsigned int.

Done.

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:112: }
On 2012/02/17 00:14:07, wtc wrote:
> 
> Nit: this file seems to omit curly braces for one-liner if
> statements, so we should follow that style.

Done.

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:368: // See bug 114185.
On 2012/02/17 00:14:07, wtc wrote:
> 
> On 2012/02/16 23:48:22, dpapad wrote:
> > Could you also remove this comment and add the bug number to the cl
> description?
> 
> I believe we need to keep the "See bug 114185." comment
> because this unit test is a regression test for that bug.
> 

I think Demetrios is right. Bug 114185 seems to be a duplicate of 40932.

http://codereview.chromium.org/9414024/diff/1/chrome/browser/ssl/ssl_browser_...
chrome/browser/ssl/ssl_browser_tests.cc:368: // See bug 114185.
On 2012/02/16 23:48:22, dpapad wrote:
> Could you also remove this comment and add the bug number to the cl
description?

Done.

[wtc, 2012-02-17 19:43:03]

Patch Set 2 LGTM.  One nit.

http://codereview.chromium.org/9414024/diff/7001/chrome/browser/ssl/ssl_brows...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/9414024/diff/7001/chrome/browser/ssl/ssl_brows...
chrome/browser/ssl/ssl_browser_tests.cc:401: // See bug 114185.

This line should also be removed.

[tpayne, 2012-02-17 19:47:38]

http://codereview.chromium.org/9414024/diff/7001/chrome/browser/ssl/ssl_brows...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

http://codereview.chromium.org/9414024/diff/7001/chrome/browser/ssl/ssl_brows...
chrome/browser/ssl/ssl_browser_tests.cc:401: // See bug 114185.
On 2012/02/17 19:43:03, wtc wrote:
> 
> This line should also be removed.

Done.

[commit-bot: I haz the power, 2012-02-17 20:51:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tpayne@chromium.org/9414024/10001

[commit-bot: I haz the power, 2012-02-17 22:10:32]

Change committed as 122600

[Ryan Sleevi, 2012-02-17 22:28:45]

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.

[tpayne, 2012-02-17 22:30:13]

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.