[Sync] USS: Add SMTP tests, clean up constants, etc.

[Sync] USS: Add SMTP tests, clean up constants, etc.

BUG=569636, 569678
Committed: https://crrev.com/988d4da2acf4861ce2e35a7d43f2c1ecff43781f
Cr-Commit-Position: refs/heads/master@{#381518}

[maxbogue, 2016-03-15 22:38:50]

maxbogue@chromium.org changed reviewers:
+ pavely@chromium.org, skym@chromium.org

[maxbogue, 2016-03-15 22:38:51]

PTAL; just adding/improving/cleaning up test things.

[skym, 2016-03-15 23:08:00]

lgtm

https://codereview.chromium.org/1800313002/diff/1/sync/internal_api/shared_mo...
File sync/internal_api/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/1800313002/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor_unittest.cc:1080: EXPECT_EQ(0U,
db().MetadataCount());
Seems like if the processor issued a ApplySyncChanges with only data deletions
in it, and metadata clears it would still pass this test. But that would be
unnecessary disk io.

Also, what about making sure data type state doesn't change?

[maxbogue, 2016-03-15 23:29:01]

https://codereview.chromium.org/1800313002/diff/1/sync/internal_api/shared_mo...
File sync/internal_api/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/1800313002/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor_unittest.cc:1080: EXPECT_EQ(0U,
db().MetadataCount());
On 2016/03/15 23:08:00, skym wrote:
> Seems like if the processor issued a ApplySyncChanges with only data deletions
> in it, and metadata clears it would still pass this test. But that would be
> unnecessary disk io.
> 
> Also, what about making sure data type state doesn't change?

I already had strict reqs for data in SMTPT::ApplySyncChanges, but apparently I
was missing the check for metadata clear in SMTPT::ApplyMetadataChangeList.
That's been added now.

Why wouldn't the DTS change? It's an update from the server; the progress
markers should be updated, no? I can check that the other fields are the same if
you'd like.

[pavely, 2016-03-16 19:22:28]

lgtm

[skym, 2016-03-16 19:25:22]

https://codereview.chromium.org/1800313002/diff/1/sync/internal_api/shared_mo...
File sync/internal_api/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/1800313002/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor_unittest.cc:1080: EXPECT_EQ(0U,
db().MetadataCount());
On 2016/03/15 23:29:01, maxbogue wrote:
> On 2016/03/15 23:08:00, skym wrote:
> > Seems like if the processor issued a ApplySyncChanges with only data
deletions
> > in it, and metadata clears it would still pass this test. But that would be
> > unnecessary disk io.
> > 
> > Also, what about making sure data type state doesn't change?
> 
> I already had strict reqs for data in SMTPT::ApplySyncChanges, but apparently
I
> was missing the check for metadata clear in SMTPT::ApplyMetadataChangeList.
> That's been added now.
> 
> Why wouldn't the DTS change? It's an update from the server; the progress
> markers should be updated, no? I can check that the other fields are the same
if
> you'd like.

Aah, you're right, the SMTPT expects things to be/not be there. Cool.

I don't think commit responses contain progress markers. We shouldn't be asking
services to save the DTS if it doesn't actually change. Which may be another
good test case for the processor. GetUpdates call completes, contains no
entities, and a new progress marker that results in the exact same DTS that we
used to have. Wouldn't make sense to save it...

[maxbogue, 2016-03-16 20:11:29]

https://codereview.chromium.org/1800313002/diff/1/sync/internal_api/shared_mo...
File sync/internal_api/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/1800313002/diff/1/sync/internal_api/shared_mo...
sync/internal_api/shared_model_type_processor_unittest.cc:1080: EXPECT_EQ(0U,
db().MetadataCount());
On 2016/03/16 19:25:21, skym wrote:
> On 2016/03/15 23:29:01, maxbogue wrote:
> > On 2016/03/15 23:08:00, skym wrote:
> > > Seems like if the processor issued a ApplySyncChanges with only data
> deletions
> > > in it, and metadata clears it would still pass this test. But that would
be
> > > unnecessary disk io.
> > > 
> > > Also, what about making sure data type state doesn't change?
> > 
> > I already had strict reqs for data in SMTPT::ApplySyncChanges, but
apparently
> I
> > was missing the check for metadata clear in SMTPT::ApplyMetadataChangeList.
> > That's been added now.
> > 
> > Why wouldn't the DTS change? It's an update from the server; the progress
> > markers should be updated, no? I can check that the other fields are the
same
> if
> > you'd like.
> 
> Aah, you're right, the SMTPT expects things to be/not be there. Cool.
> 
> I don't think commit responses contain progress markers. We shouldn't be
asking
> services to save the DTS if it doesn't actually change. Which may be another
> good test case for the processor. GetUpdates call completes, contains no
> entities, and a new progress marker that results in the exact same DTS that we
> used to have. Wouldn't make sense to save it...

Filed http://crbug.com/595427 to track adding DTS stuff to test cases.

[maxbogue, 2016-03-16 20:11:34]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-03-16 20:11:34]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/1800313002/#ps20001
(title: "Stricter ApplyMetadataChangeList.")

[commit-bot: I haz the power, 2016-03-16 20:12:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800313002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800313002/20001

[commit-bot: I haz the power, 2016-03-16 20:18:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-16 20:20:18]

Description was changed from

==========
[Sync] USS: Add SMTP tests, clean up constants, etc.

BUG=569636,569678
==========

to

==========
[Sync] USS: Add SMTP tests, clean up constants, etc.

BUG=569636,569678

Committed: https://crrev.com/988d4da2acf4861ce2e35a7d43f2c1ecff43781f
Cr-Commit-Position: refs/heads/master@{#381518}
==========

[commit-bot: I haz the power, 2016-03-16 20:20:19]

Patchset 2 (id:??) landed as
https://crrev.com/988d4da2acf4861ce2e35a7d43f2c1ecff43781f
Cr-Commit-Position: refs/heads/master@{#381518}