sync: Refactor NonBlockingTypeProcessorCore tests

sync: Refactor NonBlockingTypeProcessorCore tests

Pulls MockNonBlockingTypeProcessor and SimpleMockServer classes out of
the NonBlockingTypeProcessorCore's test harness.

These two new mock classes are currently used in only one place, though
it's probable that they'll be reused elsewhere sooner or later.  Even if
no code is reused, this change is worthwhile because it leads to better
separation of responsibilities and improved documentation.

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

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

[rlarocque, 2014-06-11 20:59:26]

Here's part two of the test refactoring.  I think these changes are independent
from the first CL, so there should be no risk of conflicts here.

Please review.

[Nicolas Zea, 2014-06-13 21:59:43]

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/mock_non_bl...
File sync/test/engine/mock_non_blocking_type_processor.cc (right):

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/mock_non_bl...
sync/test/engine/mock_non_blocking_type_processor.cc:22: base::Unretained(this),
Now that this is a standalone class, it's probably cleaner/safer to go ahead and
have a weak ptr factory

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/mock_non_bl...
File sync/test/engine/mock_non_blocking_type_processor.h (right):

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/mock_non_bl...
sync/test/engine/mock_non_blocking_type_processor.h:27: // based on their value.
nit: "on based on" -> "based on"

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/mock_non_bl...
sync/test/engine/mock_non_blocking_type_processor.h:48: void RunQueuedTasks();
Should this only be used if synchronous execution is disabled? Perhaps add a
comment to that effect?

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
File sync/test/engine/simple_mock_server.h (right):

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
sync/test/engine/simple_mock_server.h:3: // found in the LICENSE file.
no ifdef header guards? I thought the presubmit checked that.

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
sync/test/engine/simple_mock_server.h:18: class SimpleMockServer {
Are there plans to have other kinds of mock servers (i.e. non-simple)? If not,
that prefix seems unnecessary. I'm also slightly concerned about this being
confused with the fake c++ server, a comment explaining the difference might be
useful. Also, would naming this simple_fake_server make more sense?

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
sync/test/engine/simple_mock_server.h:20: SimpleMockServer(syncer::ModelType
type);
nit: explicit

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
sync/test/engine/simple_mock_server.h:84: std::map<const std::string,
sync_pb::SyncEntity> committed_items_;
disallow copy and assign

[rlarocque, 2014-06-14 00:55:19]

Here's the first round of fixes.  PTAL.

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/mock_non_bl...
File sync/test/engine/mock_non_blocking_type_processor.cc (right):

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/mock_non_bl...
sync/test/engine/mock_non_blocking_type_processor.cc:22: base::Unretained(this),
On 2014/06/13 21:59:42, Nicolas Zea wrote:
> Now that this is a standalone class, it's probably cleaner/safer to go ahead
and
> have a weak ptr factory

I don't think that's necessary.  The queue of tasks are owned by this object. 
They're never exposed to any external observers.  If 'this' is destroyed, all
the tasks that point to it will be deleted, too.

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/mock_non_bl...
File sync/test/engine/mock_non_blocking_type_processor.h (right):

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/mock_non_bl...
sync/test/engine/mock_non_blocking_type_processor.h:27: // based on their value.
On 2014/06/13 21:59:42, Nicolas Zea wrote:
> nit: "on based on" -> "based on"

Done.

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/mock_non_bl...
sync/test/engine/mock_non_blocking_type_processor.h:48: void RunQueuedTasks();
On 2014/06/13 21:59:42, Nicolas Zea wrote:
> Should this only be used if synchronous execution is disabled? Perhaps add a
> comment to that effect?

Done.

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
File sync/test/engine/simple_mock_server.h (right):

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
sync/test/engine/simple_mock_server.h:3: // found in the LICENSE file.
On 2014/06/13 21:59:43, Nicolas Zea wrote:
> no ifdef header guards? I thought the presubmit checked that.

I don't know how the presubmit missed it.  Fixed.

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
sync/test/engine/simple_mock_server.h:18: class SimpleMockServer {
On 2014/06/13 21:59:43, Nicolas Zea wrote:
> Are there plans to have other kinds of mock servers (i.e. non-simple)? If not,
> that prefix seems unnecessary. I'm also slightly concerned about this being
> confused with the fake c++ server, a comment explaining the difference might
be
> useful. Also, would naming this simple_fake_server make more sense?

There's MockConnectionManager, which probably deserves the name MockServer more
than this class does.  It does a lot more than mock connections.

I tried to explain a bit of the differences between this class and the
FakeServer in the class comment.  This class has no intention of behaving like a
real server.  The lack of support for more than one ModelType is the most
obvious missing feature.

Would it help if I renamed it to SingleTypeMockServer?  Maybe
UnitTestMockServer?  I could also rename it as SimpleFakeServer if you think
that's a better idea, but I think that would imply some sort of relationship
with the FakeServer.

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
sync/test/engine/simple_mock_server.h:20: SimpleMockServer(syncer::ModelType
type);
On 2014/06/13 21:59:43, Nicolas Zea wrote:
> nit: explicit

Done.

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
sync/test/engine/simple_mock_server.h:84: std::map<const std::string,
sync_pb::SyncEntity> committed_items_;
On 2014/06/13 21:59:42, Nicolas Zea wrote:
> disallow copy and assign

Done.

[Nicolas Zea, 2014-06-16 21:12:58]

LGTM with the naming change

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
File sync/test/engine/simple_mock_server.h (right):

https://codereview.chromium.org/330523002/diff/1/sync/test/engine/simple_mock...
sync/test/engine/simple_mock_server.h:18: class SimpleMockServer {
On 2014/06/14 00:55:19, rlarocque wrote:
> On 2014/06/13 21:59:43, Nicolas Zea wrote:
> > Are there plans to have other kinds of mock servers (i.e. non-simple)? If
not,
> > that prefix seems unnecessary. I'm also slightly concerned about this being
> > confused with the fake c++ server, a comment explaining the difference might
> be
> > useful. Also, would naming this simple_fake_server make more sense?
> 
> There's MockConnectionManager, which probably deserves the name MockServer
more
> than this class does.  It does a lot more than mock connections.
> 
> I tried to explain a bit of the differences between this class and the
> FakeServer in the class comment.  This class has no intention of behaving like
a
> real server.  The lack of support for more than one ModelType is the most
> obvious missing feature.
> 
> Would it help if I renamed it to SingleTypeMockServer?  Maybe
> UnitTestMockServer?  I could also rename it as SimpleFakeServer if you think
> that's a better idea, but I think that would imply some sort of relationship
> with the FakeServer.

I'm good with SingleTypeMockServer

[rlarocque, 2014-06-16 21:45:06]

Renamed and rebased.  Will send to the CQ.

[rlarocque, 2014-06-16 21:45:16]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-16 21:48:32]

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

[commit-bot: I haz the power, 2014-06-17 06:58:52]

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

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

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[rlarocque, 2014-06-17 17:32:12]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-17 17:34:27]

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

[commit-bot: I haz the power, 2014-06-18 01:03:36]

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

[commit-bot: I haz the power, 2014-06-18 01:03:37]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[rlarocque, 2014-06-18 17:39:50]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-18 17:41:32]

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

[commit-bot: I haz the power, 2014-06-18 17:43:17]

Change committed as 278114