HttpServerPropertiesManager and tests cleanup

HttpServerPropertiesManager and tests cleanup

This CL cleans up HttpServerPropertiesManager and its unittests:

HttpServerPropertiesManager:
1. Cleanup test-only virtual methods in cc and h files.

Tests:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
4. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.

Review-Url: https://codereview.chromium.org/2681383002
Cr-Commit-Position: refs/heads/master@{#449713}
Committed: https://chromium.googlesource.com/chromium/src/+/2b355b10cfd06d3a0e4b5fc563a3c9ee2f4e4d79

[xunjieli, 2017-02-09 18:52:54]

Interestingly, we have some commented out code in the test file. I think it is
unintentional. This is to uncomment the code.

[xunjieli, 2017-02-09 18:52:58]

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

[commit-bot: I haz the power, 2017-02-09 18:53:30]

Dry run: 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-02-09 19:40:10]

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

[commit-bot: I haz the power, 2017-02-09 19:40:11]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-02-09 19:50:08]

Description was changed from

==========
Uncommented out test code in http_server_properties_manager_unittest.cc

R=zhongyi@chromium.org
==========

to

==========
HttpServerPropertiesManager tests cleanup

This CL cleans up the unittest:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Cleanup test only methods in cc and h files.

R=zhongyi@chromium.org
==========

[xunjieli, 2017-02-09 19:50:32]

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

[commit-bot: I haz the power, 2017-02-09 19:51:08]

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

[xunjieli, 2017-02-09 19:56:12]

Patchset #2 (id:20001) has been deleted

[xunjieli, 2017-02-09 19:56:16]

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

[commit-bot: I haz the power, 2017-02-09 19:56:58]

Dry run: 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-02-09 20:30:57]

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

[commit-bot: I haz the power, 2017-02-09 20:30:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[xunjieli, 2017-02-09 21:48:21]

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

[commit-bot: I haz the power, 2017-02-09 21:48:54]

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

[xunjieli, 2017-02-09 21:50:31]

Description was changed from

==========
HttpServerPropertiesManager tests cleanup

This CL cleans up the unittest:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Cleanup test only methods in cc and h files.

R=zhongyi@chromium.org
==========

to

==========
HttpServerPropertiesManager tests cleanup

This CL cleans up the unittest:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Cleanup test only methods in cc and h files.
4. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
5. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.

R=zhongyi@chromium.org
==========

[xunjieli, 2017-02-09 21:51:57]

Description was changed from

==========
HttpServerPropertiesManager tests cleanup

This CL cleans up the unittest:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Cleanup test only methods in cc and h files.
4. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
5. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.

R=zhongyi@chromium.org
==========

to

==========
HttpServerPropertiesManager and tests cleanup

This CL cleans up HttpServerPropertiesManager and its unittests:
HttpServerPropertiesManager:
1. Cleanup test only methods in cc and h files.
tests:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
5. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.

R=zhongyi@chromium.org
==========

[xunjieli, 2017-02-09 21:52:09]

Description was changed from

==========
HttpServerPropertiesManager and tests cleanup

This CL cleans up HttpServerPropertiesManager and its unittests:
HttpServerPropertiesManager:
1. Cleanup test only methods in cc and h files.
tests:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
5. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.

R=zhongyi@chromium.org
==========

to

==========
HttpServerPropertiesManager and tests cleanup

This CL cleans up HttpServerPropertiesManager and its unittests:
HttpServerPropertiesManager:
1. Cleanup test only methods in cc and h files.
tests:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
4. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.
==========

[xunjieli, 2017-02-09 21:53:03]

Description was changed from

==========
HttpServerPropertiesManager and tests cleanup

This CL cleans up HttpServerPropertiesManager and its unittests:
HttpServerPropertiesManager:
1. Cleanup test only methods in cc and h files.
tests:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
4. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.
==========

to

==========
HttpServerPropertiesManager and tests cleanup

This CL cleans up HttpServerPropertiesManager and its unittests:

HttpServerPropertiesManager:
1. Cleanup test only methods in cc and h files.

Tests:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
4. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.
==========

[xunjieli, 2017-02-09 21:53:42]

Description was changed from

==========
HttpServerPropertiesManager and tests cleanup

This CL cleans up HttpServerPropertiesManager and its unittests:

HttpServerPropertiesManager:
1. Cleanup test only methods in cc and h files.

Tests:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
4. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.
==========

to

==========
HttpServerPropertiesManager and tests cleanup

This CL cleans up HttpServerPropertiesManager and its unittests:

HttpServerPropertiesManager:
1. Cleanup test-only virtual methods in cc and h files.

Tests:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
4. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.
==========

[commit-bot: I haz the power, 2017-02-09 22:48:44]

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

[commit-bot: I haz the power, 2017-02-09 22:48:44]

Dry run: This issue passed the CQ dry run.

[Zhongyi Shi, 2017-02-10 18:01:38]

Thanks for cleaning this up, looks good to me, just one question about the test.

https://codereview.chromium.org/2681383002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (left):

https://codereview.chromium.org/2681383002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:1178: 
Hm, I thought we had UpdateCacheWithPref and UpdatePrefsWithCache in the tests
before your change. 

The other way to make this test indeed a UpdateCacheWithPrefs would be instead
of call ScheduleUpdateCacheOnPrefThread directly, call
OnHttpServerPropertiesChanged. Does that make sense? Your fix looks good to me,
maybe we should add one more test. How do you think? 

https://cs.chromium.org/chromium/src/net/http/http_server_properties_manager....

https://codereview.chromium.org/2681383002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2681383002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:659: const int64_t
kUpdatePrefsDelayMs = 60000;
Just to confirm, looks like all the tests are now running with the default delay
as .cc file when update pref/cache, right? 

Here we need to declare a local kUpdatePrefsDelayMs as we want to control the
timer and verify the behavior of ther net thread. In those tests we don't need
verify, we just fast forward until no task remain.

[xunjieli, 2017-02-10 20:01:21]

https://codereview.chromium.org/2681383002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (left):

https://codereview.chromium.org/2681383002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:1178: 
On 2017/02/10 18:01:38, Zhongyi Shi wrote:
> Hm, I thought we had UpdateCacheWithPref and UpdatePrefsWithCache in the tests
> before your change. 
> 
> The other way to make this test indeed a UpdateCacheWithPrefs would be instead
> of call ScheduleUpdateCacheOnPrefThread directly, call
> OnHttpServerPropertiesChanged. Does that make sense? Your fix looks good to
me,
> maybe we should add one more test. How do you think? 
> 
>
https://cs.chromium.org/chromium/src/net/http/http_server_properties_manager....

Talked to Cherie offline. 
There are other 4 tests that test the other direction:
BadSupportsQuic, SingleUpdateForTwoSpdyServerPrefChanges, BadCachedHostPortPair,
BadCachedAltProtocolPort

https://codereview.chromium.org/2681383002/diff/80001/net/http/http_server_pr...
File net/http/http_server_properties_manager_unittest.cc (right):

https://codereview.chromium.org/2681383002/diff/80001/net/http/http_server_pr...
net/http/http_server_properties_manager_unittest.cc:659: const int64_t
kUpdatePrefsDelayMs = 60000;
On 2017/02/10 18:01:38, Zhongyi Shi wrote:
> Just to confirm, looks like all the tests are now running with the default
delay
> as .cc file when update pref/cache, right? 
> 
Yes.

> Here we need to declare a local kUpdatePrefsDelayMs as we want to control the
> timer and verify the behavior of ther net thread. In those tests we don't need
> verify, we just fast forward until no task remain.

Yes, that's my understanding too.

[Zhongyi Shi, 2017-02-10 20:03:36]

lgtm! 

Thanks for improving those tests!

[xunjieli, 2017-02-10 20:04:24]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2017-02-10 20:05:34]

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-02-10 20:13:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486757064842590,
"parent_rev": "b36013be8cd2518e4b41e43ace3f6007c48ea078", "commit_rev":
"2b355b10cfd06d3a0e4b5fc563a3c9ee2f4e4d79"}

[commit-bot: I haz the power, 2017-02-10 20:13:33]

Description was changed from

==========
HttpServerPropertiesManager and tests cleanup

This CL cleans up HttpServerPropertiesManager and its unittests:

HttpServerPropertiesManager:
1. Cleanup test-only virtual methods in cc and h files.

Tests:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
4. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.
==========

to

==========
HttpServerPropertiesManager and tests cleanup

This CL cleans up HttpServerPropertiesManager and its unittests:

HttpServerPropertiesManager:
1. Cleanup test-only virtual methods in cc and h files.

Tests:
1. Re-enable unintentional commented out test code 
2. Remove unused fields and methods in test file
3. Rename UpdateCacheWithPrefs test to UpdatePrefsWithCache test
   because that test is really testing cache -> pref update.
4. Simplify two unit tests: UpdatePrefsWithCache and 
   DoNotPersistExpiredOrBrokenAlternativeService.

Review-Url: https://codereview.chromium.org/2681383002
Cr-Commit-Position: refs/heads/master@{#449713}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b355b10cfd06d3a0e4b5fc563a3...
==========

[commit-bot: I haz the power, 2017-02-10 20:13:35]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2b355b10cfd06d3a0e4b5fc563a3...