[sync] Reland - Replace FakeServer's implementation with LoopbackServer invocations.

Replace FakeServer's implementation with LoopbackServer invocations.

This makes FakeServer a debugging adapter for LoopbackServer. It mostly
forwards calls to the actual implementation in the LoopbackServer but
also allows for simulating various exceptional conditions and deeper
inspection in the data flow.

BUG=651415
TEST=All tests still pass.

Review-Url: https://codereview.chromium.org/2909173003

[pastarmovj, 2017-06-06 08:46:50]

Description was changed from

==========
Replace FakeServer's implementation with LoopbackServer invocations.

This makes FakeServer a debugging adapter for LoopbackServer. It mostly
forwards calls to the actual implementation in the LoopbackServer but
also allows for simulating various exceptional conditions and deeper
ispection in the data flow.

BUG=651415
TEST=All tests still pass.
==========

to

==========
Replace FakeServer's implementation with LoopbackServer invocations.

This makes FakeServer a debugging adapter for LoopbackServer. It mostly
forwards calls to the actual implementation in the LoopbackServer but
also allows for simulating various exceptional conditions and deeper
ispection in the data flow.

BUG=651415
TEST=All tests still pass.
==========

[pastarmovj, 2017-06-06 08:46:59]

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

[commit-bot: I haz the power, 2017-06-06 08:47:09]

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-06-06 08:49:19]

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

[commit-bot: I haz the power, 2017-06-06 08:49:20]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pastarmovj, 2017-06-06 09:07:10]

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

[commit-bot: I haz the power, 2017-06-06 09:07:22]

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-06-06 10:26:48]

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

[commit-bot: I haz the power, 2017-06-06 10:26:49]

Dry run: This issue passed the CQ dry run.

[pastarmovj, 2017-06-06 10:51:57]

pastarmovj@chromium.org changed reviewers:
+ zea@chromium.org

[pastarmovj, 2017-06-06 10:51:57]

Hi Nicolas,

Finally got all tests to pass. Please take a look when you got some time. The CL
looks intimidating at first but it is mostly removal of duplicated code and some
little adaptation that was needed to make the tests compile.

The FakeServer is now an adapter over LoopbackServer which serves to handle the
mock error conditions but delegates the actual data handling to the
LoopbackServer. The loopback server had to be extended with some methods for
inspecting/changing its internal state but those are hidden only for its
"friend" the FakeServer therefore impossible to abuse in production code.

Thanks,
Julian

[Nicolas Zea, 2017-06-08 16:55:08]

Mostly LG!

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
File components/sync/engine_impl/loopback_server/loopback_server.h (right):

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/loopback_server.h:125: // (e.g., the
URL of a session tab).
Couldn't you return a LoopbackServerEntity? It provides a GetSpecifics method
which should provide any data you need, right?

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/loopback_server.h:137: // updated.
If the given |id| does not exist or the ModelType of
what is the version updated to? n+1?

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
File components/sync/engine_impl/loopback_server/persistent_tombstone_entity.h
(right):

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/persistent_tombstone_entity.h:26:
static std::unique_ptr<LoopbackServerEntity> CreateForInjection(
It's not clear to me why these two are different? Create is also for injection
too, right? Maybe distinguish in some way?

[pastarmovj, 2017-06-09 13:13:49]

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
File components/sync/engine_impl/loopback_server/loopback_server.h (right):

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/loopback_server.h:125: // (e.g., the
URL of a session tab).
On 2017/06/08 16:55:08, Nicolas Zea wrote:
> Couldn't you return a LoopbackServerEntity? It provides a GetSpecifics method
> which should provide any data you need, right?

This function is in tests as is now. I can adjust all tests that use it if you
wish but I tried to keep the change minimal in this regard.

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/loopback_server.h:137: // updated.
If the given |id| does not exist or the ModelType of
On 2017/06/08 16:55:08, Nicolas Zea wrote:
> what is the version updated to? n+1?

Yes. I updated the documentation.

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
File components/sync/engine_impl/loopback_server/persistent_tombstone_entity.h
(right):

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/persistent_tombstone_entity.h:26:
static std::unique_ptr<LoopbackServerEntity> CreateForInjection(
On 2017/06/08 16:55:08, Nicolas Zea wrote:
> It's not clear to me why these two are different? Create is also for injection
> too, right? Maybe distinguish in some way?

Actually create is the factory method used by the code handling the queries from
the sync client whereas those "...ForInjection" methods are used by tests that
manually construct entries to put in the server cache without going through the
proto format. I could of course generate the SyncEntity objects in tests but I
kept the structure as it was. Do you want me to change that?

[Nicolas Zea, 2017-06-09 19:44:10]

LGTM with some nits

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
File components/sync/engine_impl/loopback_server/loopback_server.h (right):

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/loopback_server.h:125: // (e.g., the
URL of a session tab).
On 2017/06/09 13:13:49, pastarmovj wrote:
> On 2017/06/08 16:55:08, Nicolas Zea wrote:
> > Couldn't you return a LoopbackServerEntity? It provides a GetSpecifics
method
> > which should provide any data you need, right?
> 
> This function is in tests as is now. I can adjust all tests that use it if you
> wish but I tried to keep the change minimal in this regard.

Maybe just get rid of the second sentence in the comment then. It confused me
that it implies we need a SyncEntity in order to query datatype specific state,
when a server entity works just fine.

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
File components/sync/engine_impl/loopback_server/persistent_tombstone_entity.h
(right):

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/persistent_tombstone_entity.h:26:
static std::unique_ptr<LoopbackServerEntity> CreateForInjection(
On 2017/06/09 13:13:49, pastarmovj wrote:
> On 2017/06/08 16:55:08, Nicolas Zea wrote:
> > It's not clear to me why these two are different? Create is also for
injection
> > too, right? Maybe distinguish in some way?
> 
> Actually create is the factory method used by the code handling the queries
from
> the sync client whereas those "...ForInjection" methods are used by tests that
> manually construct entries to put in the server cache without going through
the
> proto format. I could of course generate the SyncEntity objects in tests but I
> kept the structure as it was. Do you want me to change that?

How about just naming one CreateFromEntity and the other CreateFromMetadata? I'd
prefer to have the names distinguish what they do, rather than how they're
currently used in the code.

[pastarmovj, 2017-06-12 12:49:12]

Thanks! 

I am really happy this is now done! It both reduces the code duplication as well
as ensured the loopback server is tested against all client tests which will let
me sleep much better at night! :)

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
File components/sync/engine_impl/loopback_server/loopback_server.h (right):

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/loopback_server.h:125: // (e.g., the
URL of a session tab).
On 2017/06/09 19:44:10, Nicolas Zea wrote:
> On 2017/06/09 13:13:49, pastarmovj wrote:
> > On 2017/06/08 16:55:08, Nicolas Zea wrote:
> > > Couldn't you return a LoopbackServerEntity? It provides a GetSpecifics
> method
> > > which should provide any data you need, right?
> > 
> > This function is in tests as is now. I can adjust all tests that use it if
you
> > wish but I tried to keep the change minimal in this regard.
> 
> Maybe just get rid of the second sentence in the comment then. It confused me
> that it implies we need a SyncEntity in order to query datatype specific
state,
> when a server entity works just fine.

Done.

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
File components/sync/engine_impl/loopback_server/persistent_tombstone_entity.h
(right):

https://codereview.chromium.org/2909173003/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/persistent_tombstone_entity.h:26:
static std::unique_ptr<LoopbackServerEntity> CreateForInjection(
On 2017/06/09 19:44:10, Nicolas Zea wrote:
> On 2017/06/09 13:13:49, pastarmovj wrote:
> > On 2017/06/08 16:55:08, Nicolas Zea wrote:
> > > It's not clear to me why these two are different? Create is also for
> injection
> > > too, right? Maybe distinguish in some way?
> > 
> > Actually create is the factory method used by the code handling the queries
> from
> > the sync client whereas those "...ForInjection" methods are used by tests
that
> > manually construct entries to put in the server cache without going through
> the
> > proto format. I could of course generate the SyncEntity objects in tests but
I
> > kept the structure as it was. Do you want me to change that?
> 
> How about just naming one CreateFromEntity and the other CreateFromMetadata?
I'd
> prefer to have the names distinguish what they do, rather than how they're
> currently used in the code.

Done.

[pastarmovj, 2017-06-12 12:51:23]

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

[commit-bot: I haz the power, 2017-06-12 12:51:42]

Dry run: CQ is trying da patch.

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

[pastarmovj, 2017-06-12 12:51:52]

pastarmovj@chromium.org changed reviewers:
+ baxley@chromium.org

[pastarmovj, 2017-06-12 12:51:53]

baxley@chromium.org: Please review changes in ios/chrome/test

Thanks,
Julian

[commit-bot: I haz the power, 2017-06-12 14:23:16]

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

[commit-bot: I haz the power, 2017-06-12 14:23:16]

Dry run: This issue passed the CQ dry run.

[baxley, 2017-06-12 16:22:01]

On 2017/06/12 12:51:53, pastarmovj wrote:
> mailto:baxley@chromium.org: Please review changes in ios/chrome/test
> 
> Thanks,
> Julian

ios/chrome/test LGTM

Thanks!

[pastarmovj, 2017-06-13 07:23:53]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2017-06-13 07:23:54]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2909173003/#ps140001
(title: "Rename/unify persistent entries create function.")

[commit-bot: I haz the power, 2017-06-13 07:24:02]

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-06-13 07:27:34]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1497338633784700,
"parent_rev": "f88c5b041d56422d91ee46d144833efa3d152f42", "commit_rev":
"4993d7744127a9b1367c2834908d97397a372c29"}

[commit-bot: I haz the power, 2017-06-13 07:27:52]

Description was changed from

==========
Replace FakeServer's implementation with LoopbackServer invocations.

This makes FakeServer a debugging adapter for LoopbackServer. It mostly
forwards calls to the actual implementation in the LoopbackServer but
also allows for simulating various exceptional conditions and deeper
ispection in the data flow.

BUG=651415
TEST=All tests still pass.
==========

to

==========
Replace FakeServer's implementation with LoopbackServer invocations.

This makes FakeServer a debugging adapter for LoopbackServer. It mostly
forwards calls to the actual implementation in the LoopbackServer but
also allows for simulating various exceptional conditions and deeper
ispection in the data flow.

BUG=651415
TEST=All tests still pass.

Review-Url: https://codereview.chromium.org/2909173003
Cr-Commit-Position: refs/heads/master@{#478921}
Committed:
https://chromium.googlesource.com/chromium/src/+/4993d7744127a9b1367c2834908d...
==========

[commit-bot: I haz the power, 2017-06-13 07:27:53]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/4993d7744127a9b1367c2834908d...

[Henrik Grunell, 2017-06-13 09:07:47]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2938553003/ by grunell@chromium.org.

The reason for reverting is: Breaks several bots.

Example:
https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Tests/....

[pastarmovj, 2017-06-14 09:50:05]

Description was changed from

==========
Replace FakeServer's implementation with LoopbackServer invocations.

This makes FakeServer a debugging adapter for LoopbackServer. It mostly
forwards calls to the actual implementation in the LoopbackServer but
also allows for simulating various exceptional conditions and deeper
ispection in the data flow.

BUG=651415
TEST=All tests still pass.

Review-Url: https://codereview.chromium.org/2909173003
Cr-Commit-Position: refs/heads/master@{#478921}
Committed:
https://chromium.googlesource.com/chromium/src/+/4993d7744127a9b1367c2834908d...
==========

to

==========
Replace FakeServer's implementation with LoopbackServer invocations.

This makes FakeServer a debugging adapter for LoopbackServer. It mostly
forwards calls to the actual implementation in the LoopbackServer but
also allows for simulating various exceptional conditions and deeper
inspection in the data flow.

BUG=651415
TEST=All tests still pass.

Review-Url: https://codereview.chromium.org/2909173003
==========