sync: Refactor NonBlockingTypeProcessor tests

sync: Refactor NonBlockingTypeProcessor tests

Moves some test support classes out of the NonBlockingTypeProcessor's
unit test .cc files and into their own files.

Rearranges some responsibilities between the test harness and the mock
classes.  Much of the functionality related to mocking out server
behavior has been moved into the MockNonBlockingTypeProcessor.  This
makes it possible to share that code with other test harnesses.

This is one of several follow-ups to the commits that introduced the
NonBlockingTypeProcessor and NonBlockingTypeProcessorCore.

BUG=351005
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276830

[rlarocque, 2014-06-06 22:51:25]

Here's the first test refactor patch.  I plan to put together another one for
the NonBlockingTypeProcessorCore unit tests, too.  After that, we can move on to
other issues, like the grand renaming and writing more exhaustive unit tests.

Nicolas: Please review.

https://codereview.chromium.org/318193002/diff/40001/sync/test/engine/injecta...
File sync/test/engine/injectable_sync_core_proxy.h (right):

https://codereview.chromium.org/318193002/diff/40001/sync/test/engine/injecta...
sync/test/engine/injectable_sync_core_proxy.h:19: class InjectableSyncCoreProxy
: public syncer::SyncCoreProxy {
This name is a strong candidate for worst name in this patch series so far,
which is quite an achievement.

The name MockSyncCoreProxy is already used for a class defined in
non_blocking_data_type_controller_unittest.cc, so it seemed wrong to reuse it
here.

This is the best I can think of for now.  I'm open to other suggestions.  

Also, I'm hoping that this will be among the classes affected by the grand
rename we plan to implement soon.  If we had a better name for SyncCoreProxy,
then it might be easier to name this thing.

[stanisc, 2014-06-07 00:31:00]

https://codereview.chromium.org/318193002/diff/40001/sync/test/engine/mock_no...
File sync/test/engine/mock_non_blocking_type_processor_core.cc (right):

https://codereview.chromium.org/318193002/diff/40001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.cc:89: data.ctime =
base::Time::UnixEpoch() + base::TimeDelta::FromDays(1);
Do these specific ctime and mtime values matter? Could you add a comment
explaining them?

[rlarocque, 2014-06-09 17:35:38]

https://codereview.chromium.org/318193002/diff/40001/sync/test/engine/mock_no...
File sync/test/engine/mock_non_blocking_type_processor_core.cc (right):

https://codereview.chromium.org/318193002/diff/40001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.cc:89: data.ctime =
base::Time::UnixEpoch() + base::TimeDelta::FromDays(1);
On 2014/06/07 00:31:00, stanisc wrote:
> Do these specific ctime and mtime values matter? Could you add a comment
> explaining them?

They're the creation time and modification time of this item.  The choice
doesn't matter at all.  Timestamps have no effect on the system.  I think they
they could even go backwards in time and no one would notice.

I arbitrarily decided to make them increase with version number and start at a
non-zero value since I figured that would make for slightly better test data. 
This way we're more likely to notice if the value is not being updated or
somehow hard-coded to zero.

I'll add a comment above this block of code.

[Nicolas Zea, 2014-06-09 23:14:15]

https://codereview.chromium.org/318193002/diff/60001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor_unittest.cc (right):

https://codereview.chromium.org/318193002/diff/60001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor_unittest.cc:182: size_t
NonBlockingTypeProcessorTest::GetNumCommitRequestLists() {
Why keep these delegation methods around, rather than have the test perform the
call via a core getter?

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/injecta...
File sync/test/engine/injectable_sync_core_proxy.cc (right):

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/injecta...
sync/test/engine/injectable_sync_core_proxy.cc:43: new
InjectableSyncCoreProxy(processor_core_));
nit: merge onto one line?

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/injecta...
File sync/test/engine/injectable_sync_core_proxy.h (right):

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/injecta...
sync/test/engine/injectable_sync_core_proxy.h:21: explicit
InjectableSyncCoreProxy(NonBlockingTypeProcessorCoreInterface*);
nit: variable name

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/mock_no...
File sync/test/engine/mock_non_blocking_type_processor_core.h (right):

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.h:22: class
MockNonBlockingTypeProcessorCore
nit: Isn't this more a fake than a mock?

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.h:32: size_t
GetNumCommitRequestLists();
make these getters/helper (here and below) const?

[rlarocque, 2014-06-10 00:53:23]

Patch updated.  PTAL.

https://codereview.chromium.org/318193002/diff/60001/sync/engine/non_blocking...
File sync/engine/non_blocking_type_processor_unittest.cc (right):

https://codereview.chromium.org/318193002/diff/60001/sync/engine/non_blocking...
sync/engine/non_blocking_type_processor_unittest.cc:182: size_t
NonBlockingTypeProcessorTest::GetNumCommitRequestLists() {
On 2014/06/09 23:14:15, Nicolas Zea wrote:
> Why keep these delegation methods around, rather than have the test perform
the
> call via a core getter?

Slightly better abstraction.

It's a trade-off between slightly more boilerplate and the advantage that tests
do not depend directly on the mock_processor_core_.  If we can keep
mock_processor_core_ private, we can replace it later on with minimal effort.

Hopefully the number of tests will far exceed the number of methods that need to
be wrapped, so the cost here should pay off fairly quickly.

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/injecta...
File sync/test/engine/injectable_sync_core_proxy.cc (right):

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/injecta...
sync/test/engine/injectable_sync_core_proxy.cc:43: new
InjectableSyncCoreProxy(processor_core_));
On 2014/06/09 23:14:15, Nicolas Zea wrote:
> nit: merge onto one line?

Won't fit.  It's 81 chars.

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/injecta...
File sync/test/engine/injectable_sync_core_proxy.h (right):

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/injecta...
sync/test/engine/injectable_sync_core_proxy.h:21: explicit
InjectableSyncCoreProxy(NonBlockingTypeProcessorCoreInterface*);
On 2014/06/09 23:14:15, Nicolas Zea wrote:
> nit: variable name

Done.

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/mock_no...
File sync/test/engine/mock_non_blocking_type_processor_core.h (right):

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.h:22: class
MockNonBlockingTypeProcessorCore
On 2014/06/09 23:14:15, Nicolas Zea wrote:
> nit: Isn't this more a fake than a mock?

I honestly don't know. 
http://stackoverflow.com/questions/346372/whats-the-difference-between-faking...
offers several conflicting definitions of those terms.  Can you recommend a
specific set of terminology?

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.h:32: size_t
GetNumCommitRequestLists();
On 2014/06/09 23:14:15, Nicolas Zea wrote:
> make these getters/helper (here and below) const?

Done.

[rlarocque, 2014-06-11 20:47:52]

ping.

[Nicolas Zea, 2014-06-11 20:52:31]

LGTM with some nits/comments

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/mock_no...
File sync/test/engine/mock_non_blocking_type_processor_core.h (right):

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.h:22: class
MockNonBlockingTypeProcessorCore
On 2014/06/10 00:53:23, rlarocque wrote:
> On 2014/06/09 23:14:15, Nicolas Zea wrote:
> > nit: Isn't this more a fake than a mock?
> 
> I honestly don't know. 
>
http://stackoverflow.com/questions/346372/whats-the-difference-between-faking...
> offers several conflicting definitions of those terms.  Can you recommend a
> specific set of terminology?

What I've used, and seen used within Chrome, is that Mocks refer generally to
GMock style usage, i.e. set expectations and verify them. Fakes on the other
hand act like real implementations, just dumbed down, allowing the test to do
whatever verification it wants. Don't feel too strongly about it though, so it's
up to you.

https://codereview.chromium.org/318193002/diff/80001/sync/test/engine/mock_no...
File sync/test/engine/mock_non_blocking_type_processor_core.h (right):

https://codereview.chromium.org/318193002/diff/80001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.h:48: int64
version_offset,
Can these three methods also be const?

https://codereview.chromium.org/318193002/diff/80001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.h:62: private:
disallow copy and assign?

[rlarocque, 2014-06-11 20:57:36]

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/mock_no...
File sync/test/engine/mock_non_blocking_type_processor_core.h (right):

https://codereview.chromium.org/318193002/diff/60001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.h:22: class
MockNonBlockingTypeProcessorCore
On 2014/06/11 20:52:31, Nicolas Zea wrote:
> On 2014/06/10 00:53:23, rlarocque wrote:
> > On 2014/06/09 23:14:15, Nicolas Zea wrote:
> > > nit: Isn't this more a fake than a mock?
> > 
> > I honestly don't know. 
> >
>
http://stackoverflow.com/questions/346372/whats-the-difference-between-faking...
> > offers several conflicting definitions of those terms.  Can you recommend a
> > specific set of terminology?
> 
> What I've used, and seen used within Chrome, is that Mocks refer generally to
> GMock style usage, i.e. set expectations and verify them. Fakes on the other
> hand act like real implementations, just dumbed down, allowing the test to do
> whatever verification it wants. Don't feel too strongly about it though, so
it's
> up to you.

I think I'll leave it as "Mock" for now.  We can reconsider as we rename
NonBlockingTypeProcessorCore.

https://codereview.chromium.org/318193002/diff/80001/sync/test/engine/mock_no...
File sync/test/engine/mock_non_blocking_type_processor_core.h (right):

https://codereview.chromium.org/318193002/diff/80001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.h:48: int64
version_offset,
On 2014/06/11 20:52:31, Nicolas Zea wrote:
> Can these three methods also be const?

No.  They update internal state.

https://codereview.chromium.org/318193002/diff/80001/sync/test/engine/mock_no...
sync/test/engine/mock_non_blocking_type_processor_core.h:62: private:
On 2014/06/11 20:52:31, Nicolas Zea wrote:
> disallow copy and assign?

Done.

[rlarocque, 2014-06-11 20:58:02]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:01:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/318193002/100001

[commit-bot: I haz the power, 2014-06-11 22:20:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15639)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18776)

[commit-bot: I haz the power, 2014-06-11 22:23:06]

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

[commit-bot: I haz the power, 2014-06-11 22:23:06]

Try jobs failed on following builders:
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15713)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18846)

[rlarocque, 2014-06-12 17:50:23]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-12 17:53:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/318193002/100001

[commit-bot: I haz the power, 2014-06-12 22:27:15]

Change committed as 276830