|
|
Created:
8 years, 10 months ago by tpayne Modified:
8 years, 10 months ago CC:
chromium-reviews, vandebo (ex-Chrome) Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix 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 #Messages
Total messages: 12 (0 generated)
Do you have an example of it failing (i.e. the failing values)?
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)
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?
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.
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.
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.
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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tpayne@chromium.org/9414024/10001
Change committed as 122600
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.
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. |