Fix flaky CertificateReportingService browser tests.

Fix flaky CertificateReportingService browser tests.

CertificateReportingService browser tests wait for the IO thread
after sending a report or resetting the service. This doesn't seem
reliable and the tests occasionally fail. A better approach is to:
- For reports, wait for requests to be destroyed
(CertificateReportingService unit test already does this).
- For service resets, wait for a callback to be called.

This CL does these: It removes WaitForIOThread calls from the browser
test and replaces them with the above. It also refactors the following:
- RequestObserver is shared between browser and unit tests.
- NetworkDelegate is no longer used in the unit tests. RequestObserver is
used instead.
- All tests now expect no URL requests to remain during tear down.
- The code to check expected reports is now shared.

BUG=677560
Review-Url: https://codereview.chromium.org/2605403002
Cr-Commit-Position: refs/heads/master@{#442008}
Committed: https://chromium.googlesource.com/chromium/src/+/ba3bd9c5fe080d75f8d451709dc42372b9083daa

[meacer, 2017-01-04 00:05:10]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 00:05:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meacer, 2017-01-04 00:14:25]

Description was changed from

==========
Fix flaky CertificateReportingService browser tests.


BUG=
==========

to

==========
Fix flaky CertificateReportingService browser tests.

CertificateReportingService browser tests wait for the IO thread
after sending a report or resetting the service. This doesn't seem
reliable and the tests occasionally fail. A better approach is to:
- For reports, wait for requests to be destroyed
(CertificateReportingService unit test already does this).
- For service resets, wait for a callback to be called.

This CL does these: It removes WaitForIOThread calls from the browser
test and replaces them with the above. It also refactors the following:
- RequestObserver is shared between browser and unit tests.
- NetworkDelegate is no longer used in the unit tests. RequestObserver is
used instead.
- All tests now expect no URL requests to remain during tear down.
- The code to check expected reports is now shared.

BUG=677560
==========

[meacer, 2017-01-04 00:15:04]

meacer@chromium.org changed reviewers:
+ estark@chromium.org, jialiul@chromium.org

[meacer, 2017-01-04 00:15:05]

jialiul, estark: Can you please take a look?

Thanks.

[commit-bot: I haz the power, 2017-01-04 00:57:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 00:57:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Jialiu Lin, 2017-01-04 01:10:37]

Seems some bots are not happy. 
LGTM in general.

https://codereview.chromium.org/2605403002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc
(right):

https://codereview.chromium.org/2605403002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:195:
unsigned int num_expected_failed_report_ = -1;
Can I unsigned int be negative? Why cannot it be initialized to 0?

[meacer, 2017-01-04 01:49:13]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[meacer, 2017-01-04 01:49:21]

https://codereview.chromium.org/2605403002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc
(right):

https://codereview.chromium.org/2605403002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:195:
unsigned int num_expected_failed_report_ = -1;
On 2017/01/04 01:10:37, Jialiu Lin wrote:
> Can I unsigned int be negative? Why cannot it be initialized to 0?

Nope, the bots were complaining about this :)

It's initialized to -1 to make sure the test cases override it (this is checked
in line 92)

[commit-bot: I haz the power, 2017-01-04 01:49:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-01-04 01:52:52]

On 2017/01/04 01:49:21, Mustafa Emre Acer wrote:
>
https://codereview.chromium.org/2605403002/diff/40001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/2605403002/diff/40001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:195:
> unsigned int num_expected_failed_report_ = -1;
> On 2017/01/04 01:10:37, Jialiu Lin wrote:
> > Can I unsigned int be negative? Why cannot it be initialized to 0?
> 
> Nope, the bots were complaining about this :)
> 
> It's initialized to -1 to make sure the test cases override it (this is
checked
> in line 92)

hmm, then you probably need an additional boolean to indicate it has been set.

[commit-bot: I haz the power, 2017-01-04 02:35:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 02:35:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[meacer, 2017-01-04 18:44:00]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 18:44:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meacer, 2017-01-04 18:45:38]

On 2017/01/04 01:52:52, Jialiu Lin wrote:
> On 2017/01/04 01:49:21, Mustafa Emre Acer wrote:
> >
>
https://codereview.chromium.org/2605403002/diff/40001/chrome/browser/safe_bro...
> > File
chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2605403002/diff/40001/chrome/browser/safe_bro...
> >
chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:195:
> > unsigned int num_expected_failed_report_ = -1;
> > On 2017/01/04 01:10:37, Jialiu Lin wrote:
> > > Can I unsigned int be negative? Why cannot it be initialized to 0?
> > 
> > Nope, the bots were complaining about this :)
> > 
> > It's initialized to -1 to make sure the test cases override it (this is
> checked
> > in line 92)
> 
> hmm, then you probably need an additional boolean to indicate it has been set.

I was using the negative value as the "not set" value instead of keeping a
separate bool (assuming the histogram count will always be greater than or equal
to zero). Or perhaps I'm misunderstanding what you meant?

[Jialiu Lin, 2017-01-04 18:48:11]

On 2017/01/04 18:45:38, Mustafa Emre Acer wrote:
> On 2017/01/04 01:52:52, Jialiu Lin wrote:
> > On 2017/01/04 01:49:21, Mustafa Emre Acer wrote:
> > >
> >
>
https://codereview.chromium.org/2605403002/diff/40001/chrome/browser/safe_bro...
> > > File
> chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2605403002/diff/40001/chrome/browser/safe_bro...
> > >
> chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:195:
> > > unsigned int num_expected_failed_report_ = -1;
> > > On 2017/01/04 01:10:37, Jialiu Lin wrote:
> > > > Can I unsigned int be negative? Why cannot it be initialized to 0?
> > > 
> > > Nope, the bots were complaining about this :)
> > > 
> > > It's initialized to -1 to make sure the test cases override it (this is
> > checked
> > > in line 92)
> > 
> > hmm, then you probably need an additional boolean to indicate it has been
set.
> 
> I was using the negative value as the "not set" value instead of keeping a
> separate bool (assuming the histogram count will always be greater than or
equal
> to zero). Or perhaps I'm misunderstanding what you meant?
Changing it to int sounds good to me. :-) I don't have further comments.Thanks!

[commit-bot: I haz the power, 2017-01-04 19:43:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 19:43:32]

Dry run: This issue passed the CQ dry run.

[estark, 2017-01-05 16:42:29]

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.h (right):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:156: const
base::Callback<void()>& reset_callback);
Instead of passing this to the constructor, would it work to have a separate
SetResetCallbackForTesting() method? (so that it's clear that it's used only for
tests?)

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:230: // Called when
the service is reset.
nit: add "Used for testing"?

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc
(right):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:82:
CertificateReportingServiceFactory::GetInstance()->SetServiceResetCallback(
Can this method be SetServiceResetCallbackForTesting?

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc
(left):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:284: //
Changes the behavior of report uploads to fail or succeed.
Did you mean to delete this?

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h
(left):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:31: //
WaitForRequestDeletions(
This example still seems useful, except it should just be changed to
WaitForRequestsDestroyed?

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h
(right):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:63: void
Wait(int num_events_to_wait_for);
nit: unsigned?

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:77: int
num_events_to_wait_for_;
nit: unsigned and on line 78 too

[meacer, 2017-01-05 20:15:46]

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.h (right):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:156: const
base::Callback<void()>& reset_callback);
On 2017/01/05 16:42:28, estark (slow thru Jan 6) wrote:
> Instead of passing this to the constructor, would it work to have a separate
> SetResetCallbackForTesting() method? (so that it's clear that it's used only
for
> tests?)

That used to be the case for most of the parameters of this class, but I wanted
to avoid that and push them to the factory instead.

The main reason was it became difficult to maintain all the different
parameters, but there is another reason here: The browser tests need to be able
to set this parameter before the service is created so that they can wait for
full initialization of the service before starting to send reports.

So adding a SetResetCallbackForTesting wouldn't work for that specific scenario
where the service isn't yet created.

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:230: // Called when
the service is reset.
On 2017/01/05 16:42:28, estark (slow thru Jan 6) wrote:
> nit: add "Used for testing"?

Done.

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc
(right):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc:82:
CertificateReportingServiceFactory::GetInstance()->SetServiceResetCallback(
On 2017/01/05 16:42:28, estark (slow thru Jan 6) wrote:
> Can this method be SetServiceResetCallbackForTesting?

Done.

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc
(left):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:284: //
Changes the behavior of report uploads to fail or succeed.
On 2017/01/05 16:42:29, estark (slow thru Jan 6) wrote:
> Did you mean to delete this?

Yes, this is already documented in the header.

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h
(left):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:31: //
WaitForRequestDeletions(
On 2017/01/05 16:42:29, estark (slow thru Jan 6) wrote:
> This example still seems useful, except it should just be changed to
> WaitForRequestsDestroyed?

Done.

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h
(right):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:63: void
Wait(int num_events_to_wait_for);
On 2017/01/05 16:42:29, estark (slow thru Jan 6) wrote:
> nit: unsigned?

Done.

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:77: int
num_events_to_wait_for_;
On 2017/01/05 16:42:29, estark (slow thru Jan 6) wrote:
> nit: unsigned and on line 78 too

Done.

[estark, 2017-01-06 15:41:30]

lgtm

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.h (right):

https://codereview.chromium.org/2605403002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:156: const
base::Callback<void()>& reset_callback);
On 2017/01/05 20:15:46, Mustafa Emre Acer wrote:
> On 2017/01/05 16:42:28, estark (slow thru Jan 6) wrote:
> > Instead of passing this to the constructor, would it work to have a separate
> > SetResetCallbackForTesting() method? (so that it's clear that it's used only
> for
> > tests?)
> 
> That used to be the case for most of the parameters of this class, but I
wanted
> to avoid that and push them to the factory instead.
> 
> The main reason was it became difficult to maintain all the different
> parameters, but there is another reason here: The browser tests need to be
able
> to set this parameter before the service is created so that they can wait for
> full initialization of the service before starting to send reports.
> 
> So adding a SetResetCallbackForTesting wouldn't work for that specific
scenario
> where the service isn't yet created. 
> 

Acknowledged.

[meacer, 2017-01-06 18:27:27]

The CQ bit was checked by meacer@chromium.org

[meacer, 2017-01-06 18:27:27]

The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2605403002/#ps100001
(title: "estark comments")

[commit-bot: I haz the power, 2017-01-06 18:28:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 19:30:30]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483727247341480,
"parent_rev": "e6f55074f173bd0c45d96e98a7d747032ab786bb", "commit_rev":
"ba3bd9c5fe080d75f8d451709dc42372b9083daa"}

[commit-bot: I haz the power, 2017-01-06 19:30:59]

Description was changed from

==========
Fix flaky CertificateReportingService browser tests.

CertificateReportingService browser tests wait for the IO thread
after sending a report or resetting the service. This doesn't seem
reliable and the tests occasionally fail. A better approach is to:
- For reports, wait for requests to be destroyed
(CertificateReportingService unit test already does this).
- For service resets, wait for a callback to be called.

This CL does these: It removes WaitForIOThread calls from the browser
test and replaces them with the above. It also refactors the following:
- RequestObserver is shared between browser and unit tests.
- NetworkDelegate is no longer used in the unit tests. RequestObserver is
used instead.
- All tests now expect no URL requests to remain during tear down.
- The code to check expected reports is now shared.

BUG=677560
==========

to

==========
Fix flaky CertificateReportingService browser tests.

CertificateReportingService browser tests wait for the IO thread
after sending a report or resetting the service. This doesn't seem
reliable and the tests occasionally fail. A better approach is to:
- For reports, wait for requests to be destroyed
(CertificateReportingService unit test already does this).
- For service resets, wait for a callback to be called.

This CL does these: It removes WaitForIOThread calls from the browser
test and replaces them with the above. It also refactors the following:
- RequestObserver is shared between browser and unit tests.
- NetworkDelegate is no longer used in the unit tests. RequestObserver is
used instead.
- All tests now expect no URL requests to remain during tear down.
- The code to check expected reports is now shared.

BUG=677560

Review-Url: https://codereview.chromium.org/2605403002
Cr-Commit-Position: refs/heads/master@{#442008}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba3bd9c5fe080d75f8d451709dc4...
==========

[commit-bot: I haz the power, 2017-01-06 19:31:00]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ba3bd9c5fe080d75f8d451709dc4...