App list sync unit tests

Tests that validate the merge and process functionality of 
AppListSyncableService with various kinds of good and bad data.

BUG=645137
Review-Url: https://codereview.chromium.org/2952463002
Cr-Commit-Position: refs/heads/master@{#482092}
Committed: https://chromium.googlesource.com/chromium/src/+/a2a65e1fa1e5c88feb43b1d0b3264c3764d0481b

[rcui, 2017-06-20 19:52:18]

Description was changed from

==========
App list sync unit tests

BUG=645137
==========

to

==========
Tests that validate the merge and process functionality of
AppListSyncableService with various kinds of good and bad data.

BUG=645137
==========

[rcui, 2017-06-20 19:52:18]

rcui@chromium.org changed reviewers:
+ jennyz@chromium.org, khmel@chromium.org, xiyuan@chromium.org

[xiyuan, 2017-06-21 15:59:19]

xiyuan@chromium.org changed reviewers:
+ stevenjb@chromium.org

[xiyuan, 2017-06-21 15:59:20]

+stevenjb is probably the best person to review app list sync  related code.

Please format CL description properly. That is, having a subject line < 72 chars
and wrap long lines around 75-80 chars.

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:54: static
const std::string UNSET = "__unset__";
Global variables should only be POD.
See
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

Use constexpr when possible. And const name should be like kConstName.

i.e.
  constexpr char kUnset[] = "__unset__";

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:164:
std::string id) {
std::string -> const std::string&

And can this be a const method?

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:240: new
syncer::SyncErrorFactoryMock));
nit: use base::MakeUnique<T>() instead of std::unique_ptr<T>(new T)

[stevenjb, 2017-06-21 23:27:17]

This is great, thanks for adding the additional test coverage!

Definitely wait for khmel@, but generally looks good!

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:95:
"$$invalid_ordinal$$", "$$invalid_ordinal$$"));
This should probably also be a constant

[khmel, 2017-06-22 00:10:12]

Thanks for taking this problem!

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:93:
sync_list.push_back(CreateAppRemoteData(
In case id == UNSET we create data with different ids, EMPTY_ITEM_NAME_ID,
EMPTY_ITEM_NAME_UNSET_ID, .... 
But in case we provide id != UNSET, we create data items with the same id. Is it
expected? It looks inconsistent

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:169:
std::unique_ptr<app_list::AppListSyncableService> app_list_syncable_service_;
nit: can we leave it private and add getter:?

 protected:
  app_list::AppListSyncableService* app_list_syncable_service() { return
app_list_syncable_service_.get(); }

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:231:
"item_id1", "item_name1", "parent_id1", "ordinal", "pinordinal"));
nit: as a valid ordinal, can we use something from ordinal?
 for example: syncer::StringOrdinal::CreateInitialOrdinal()

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:231:
"item_id1", "item_name1", "parent_id1", "ordinal", "pinordinal"));
nit: as an valid item id we probably better use id of required format.
AppListSyncService works with this, but some other code may have
DCHECK(crx_file::id_util::IdIsValid(app_id));

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:270:
EXPECT_TRUE(GetSyncItem(INVALID_ORDINALS_ID));
This should be ASSERT_TRUE(GetSyncItem(INVALID_ORDINALS_ID))
In case EXPECT_TRUE and GetSyncItem(INVALID_ORDINALS_ID) is nullptr this test
would crash on next line. In case ASSERT_* test would be stopped immediately.
nit: You may probably want to use local var:
const app_list::AppListSyncableService::SyncItem* item =
etSyncItem(INVALID_ORDINALS_ID);
ASSERT_TRUE(item)
...

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:368:
syncer::SyncDataList update_list = CreateBadAppRemoteData("item_id1");
It is not clear what we want to achieve by sending CreateBadAppRemoteData(). At
line 375 we add one more change and starting from 387 we just validate that item
was set from last update. IIUC removing lines 368-373 would not affect your
test. 
Probably you may want to move L375 before L368, but I am guessing here. Could
you please add more comments before test?

[rcui, 2017-06-23 02:03:49]

Thanks for the review!  New patchset uploaded - ptal.

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:54: static
const std::string UNSET = "__unset__";
On 2017/06/21 15:59:20, xiyuan wrote:
> Global variables should only be POD.
> See
> https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
> 
> Use constexpr when possible. And const name should be like kConstName.
> 
> i.e.
>   constexpr char kUnset[] = "__unset__";

Done, thanks.  Looking a lot better now :)

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:93:
sync_list.push_back(CreateAppRemoteData(
On 2017/06/22 00:10:11, khmel wrote:
> In case id == UNSET we create data with different ids, EMPTY_ITEM_NAME_ID,
> EMPTY_ITEM_NAME_UNSET_ID, .... 
> But in case we provide id != UNSET, we create data items with the same id. Is
it
> expected? It looks inconsistent

Good point.  I'm using a different constant for this now: kDefault.

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:95:
"$$invalid_ordinal$$", "$$invalid_ordinal$$"));
On 2017/06/21 23:27:17, stevenjb wrote:
> This should probably also be a constant

Since it's only used in this function, I think it's ok to keep as-is. Imo string
literals are preferred over constants in tests b/c they are easier to read and
validate (no indirection).  Let me know if you disagree strongly.

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:164:
std::string id) {
On 2017/06/21 15:59:20, xiyuan wrote:
> std::string -> const std::string&
> 
> And can this be a const method?

Done.

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:169:
std::unique_ptr<app_list::AppListSyncableService> app_list_syncable_service_;
On 2017/06/22 00:10:11, khmel wrote:
> nit: can we leave it private and add getter:?
> 
>  protected:
>   app_list::AppListSyncableService* app_list_syncable_service() { return
> app_list_syncable_service_.get(); } 

Done.

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:231:
"item_id1", "item_name1", "parent_id1", "ordinal", "pinordinal"));
On 2017/06/22 00:10:11, khmel wrote:
> nit: as an valid item id we probably better use id of required format.
> AppListSyncService works with this, but some other code may have
> DCHECK(crx_file::id_util::IdIsValid(app_id));
> 

Done.  Id's are generated with crx_file::id_util::GenerateId()

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:231:
"item_id1", "item_name1", "parent_id1", "ordinal", "pinordinal"));
On 2017/06/22 00:10:12, khmel wrote:
> nit: as a valid ordinal, can we use something from ordinal?
>  for example: syncer::StringOrdinal::CreateInitialOrdinal()

The ordinals we use now are valid.  They also make the tests more readable I
think, so I'd prefer to keep them.

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:240: new
syncer::SyncErrorFactoryMock));
On 2017/06/21 15:59:20, xiyuan wrote:
> nit: use base::MakeUnique<T>() instead of std::unique_ptr<T>(new T)

Done.

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:270:
EXPECT_TRUE(GetSyncItem(INVALID_ORDINALS_ID));
On 2017/06/22 00:10:12, khmel wrote:
> This should be ASSERT_TRUE(GetSyncItem(INVALID_ORDINALS_ID))
> In case EXPECT_TRUE and GetSyncItem(INVALID_ORDINALS_ID) is nullptr this test
> would crash on next line. In case ASSERT_* test would be stopped immediately.
> nit: You may probably want to use local var:
> const app_list::AppListSyncableService::SyncItem* item =
> etSyncItem(INVALID_ORDINALS_ID);
> ASSERT_TRUE(item)
> ...

Done.  I passed on using a local var as it breaks the code flow and is harder to
write..

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:368:
syncer::SyncDataList update_list = CreateBadAppRemoteData("item_id1");
On 2017/06/22 00:10:12, khmel wrote:
> It is not clear what we want to achieve by sending CreateBadAppRemoteData().
At
> line 375 we add one more change and starting from 387 we just validate that
item
> was set from last update. IIUC removing lines 368-373 would not affect your
> test. 
> Probably you may want to move L375 before L368, but I am guessing here. Could
> you please add more comments before test?

I've removed the bookmark.  This test should just be validating that bad data
can be processed without crashing.

[rcui, 2017-06-23 02:05:03]

Description was changed from

==========
Tests that validate the merge and process functionality of
AppListSyncableService with various kinds of good and bad data.

BUG=645137
==========

to

==========
Tests that validate the merge and process functionality of 
AppListSyncableService with various kinds of good and bad data.

BUG=645137
==========

[xiyuan, 2017-06-23 17:11:34]

https://codereview.chromium.org/2952463002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:61: const
std::string kInvalidOrdinalsId = GenerateId("invalid_ordinals");
std::string is not allowed for global variable and should not be initialized
with a function call.

[stevenjb, 2017-06-23 19:46:05]

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:95:
"$$invalid_ordinal$$", "$$invalid_ordinal$$"));
On 2017/06/23 02:03:48, rcui wrote:
> On 2017/06/21 23:27:17, stevenjb wrote:
> > This should probably also be a constant
> 
> Since it's only used in this function, I think it's ok to keep as-is. Imo
string
> literals are preferred over constants in tests b/c they are easier to read and
> validate (no indirection).  Let me know if you disagree strongly.

Acknowledged.

https://codereview.chromium.org/2952463002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:61: const
std::string kInvalidOrdinalsId = GenerateId("invalid_ordinals");
On 2017/06/23 17:11:34, xiyuan wrote:
> std::string is not allowed for global variable and should not be initialized
> with a function call.

e.g.. these could be members of a helper struct and initialized in the
constructor. The helper can be owned by AppListSyncableServiceTest and passed to
CreateBadAppRemoteData.

Alternatively these can all be constexpr char[] and GenerateId() can be called
where they are used.

[rcui, 2017-06-23 21:28:08]

https://codereview.chromium.org/2952463002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:61: const
std::string kInvalidOrdinalsId = GenerateId("invalid_ordinals");
On 2017/06/23 19:46:05, stevenjb wrote:
> On 2017/06/23 17:11:34, xiyuan wrote:
> > std::string is not allowed for global variable and should not be initialized
> > with a function call.
> 
> e.g.. these could be members of a helper struct and initialized in the
> constructor. The helper can be owned by AppListSyncableServiceTest and passed
to
> CreateBadAppRemoteData.
> 
> Alternatively these can all be constexpr char[] and GenerateId() can be called
> where they are used.

How about this - defining the constants as functions and keeping the naming
scheme.  Adds extra overhead to the tests, but there's no loop-nesting going on,
so the runtime impact is pretty minimal.

[rcui, 2017-06-23 21:28:42]

https://codereview.chromium.org/2952463002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:61: const
std::string kInvalidOrdinalsId = GenerateId("invalid_ordinals");
On 2017/06/23 21:28:08, rcui wrote:
> On 2017/06/23 19:46:05, stevenjb wrote:
> > On 2017/06/23 17:11:34, xiyuan wrote:
> > > std::string is not allowed for global variable and should not be
initialized
> > > with a function call.
> > 
> > e.g.. these could be members of a helper struct and initialized in the
> > constructor. The helper can be owned by AppListSyncableServiceTest and
passed
> to
> > CreateBadAppRemoteData.
> > 
> > Alternatively these can all be constexpr char[] and GenerateId() can be
called
> > where they are used.
> 
> How about this - defining the constants as functions and keeping the naming
> scheme.  Adds extra overhead to the tests, but there's no loop-nesting going
on,
> so the runtime impact is pretty minimal.

This is done in the newest patchset.

[stevenjb, 2017-06-23 21:29:50]

lgtm

[xiyuan, 2017-06-23 22:31:19]

lgtm

[khmel, 2017-06-23 22:37:27]

lgtm

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:61: const
std::string kInvalidOrdinalsId() {
nit: it seems const here does not make sense. You return not reference. Also k
looks strange. Can't find this pattern in the code.

How about:
std::string InvalidOrdinalsId() ?

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:291:
syncer::SyncDataList sync_list = CreateBadAppRemoteData(kDefault);
nit: const syncer::...

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:301:
EXPECT_EQ("n",
nit: It is not clear that "n" represents initial value. Please use value
returned by creating initial ordinal or add comment
you may do something const std::string initial_ordinal = ....
and substitute all "n". I am also ok with comment.

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:370:
EXPECT_EQ("item_name1x", GetSyncItem(kItemId1)->item_name);
nit: It seems you frequently use this pattern. If you want you may add some
helper protected method (not forcing to do this):

void ValidateSyncItem(const std::string& id,
                      const std::string& expected_name,....) {
...
}

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:412:
ASSERT_TRUE(GetSyncItem(kItemId));
nit: it would be nice to check that sync item was not modified after applying
bad data.

[rcui, 2017-06-23 22:37:47]

The CQ bit was checked by rcui@chromium.org

[rcui, 2017-06-23 22:37:59]

The CQ bit was unchecked by rcui@chromium.org

[rcui, 2017-06-23 22:39:48]

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

[commit-bot: I haz the power, 2017-06-23 22:40:05]

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-23 23:01:39]

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

[commit-bot: I haz the power, 2017-06-23 23:01:40]

Dry run: This issue passed the CQ dry run.

[rcui, 2017-06-24 00:19:11]

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:61: const
std::string kInvalidOrdinalsId() {
On 2017/06/23 22:37:27, khmel wrote:
> nit: it seems const here does not make sense. You return not reference. Also k
> looks strange. Can't find this pattern in the code.
> 
> How about:
> std::string InvalidOrdinalsId() ?

Keeping the const, as it maintains consistency with how a regular global
constant works.  The 'k' naming convention is also to maintain consistency with
regular constants - I'd prefer to keep this as it makes reading the code that
uses the constant easier - 'kInvalidOrdinalsId()' is quickly grokked as a
constant.

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:291:
syncer::SyncDataList sync_list = CreateBadAppRemoteData(kDefault);
On 2017/06/23 22:37:27, khmel wrote:
> nit: const syncer::...

Done.

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:301:
EXPECT_EQ("n",
On 2017/06/23 22:37:27, khmel wrote:
> nit: It is not clear that "n" represents initial value. Please use value
> returned by creating initial ordinal or add comment
> you may do something const std::string initial_ordinal = ....
> and substitute all "n". I am also ok with comment.

With tests, I think it's good to be as literal as possible when validating
results and side effects.  I've added comments indicating that we expect the
item_ordinal to be fixed up.

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:370:
EXPECT_EQ("item_name1x", GetSyncItem(kItemId1)->item_name);
On 2017/06/23 22:37:27, khmel wrote:
> nit: It seems you frequently use this pattern. If you want you may add some
> helper protected method (not forcing to do this):
> 
> void ValidateSyncItem(const std::string& id,
>                       const std::string& expected_name,....) {
> ...
> }

I went ahead and tried it, and the issue with this approach is it's hard to see
which part of the test the failed expectation (EXPECT, ASSERT) belongs to,
because all failures are reported as happening in ValidateSyncItem rather than
its caller, and passing --gtest_throw_on_failure to the unit_test runner doesn't
work either.  Given it's a nice-to-have, I will go ahead without this.  Would be
happy to hear ways around this if you know of any.

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:412:
ASSERT_TRUE(GetSyncItem(kItemId));
On 2017/06/23 22:37:27, khmel wrote:
> nit: it would be nice to check that sync item was not modified after applying
> bad data.

Given the type of bad data we're using, the sync item would be modified, no?

[rcui, 2017-06-24 00:19:42]

The CQ bit was checked by rcui@chromium.org

[rcui, 2017-06-24 00:19:42]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
stevenjb@chromium.org, khmel@chromium.org
Link to the patchset: https://codereview.chromium.org/2952463002/#ps80001
(title: "App list sync unit tests")

[commit-bot: I haz the power, 2017-06-24 00:20:05]

CQ is trying da patch.

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

[khmel, 2017-06-24 00:30:16]

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:61: const
std::string kInvalidOrdinalsId() {
On 2017/06/24 00:19:10, rcui wrote:
> On 2017/06/23 22:37:27, khmel wrote:
> > nit: it seems const here does not make sense. You return not reference. Also
k
> > looks strange. Can't find this pattern in the code.
> > 
> > How about:
> > std::string InvalidOrdinalsId() ?
> 
> Keeping the const, as it maintains consistency with how a regular global
> constant works.  The 'k' naming convention is also to maintain consistency
with
> regular constants - I'd prefer to keep this as it makes reading the code that
> uses the constant easier - 'kInvalidOrdinalsId()' is quickly grokked as a
> constant.

Acknowledged.

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:301:
EXPECT_EQ("n",
On 2017/06/24 00:19:10, rcui wrote:
> On 2017/06/23 22:37:27, khmel wrote:
> > nit: It is not clear that "n" represents initial value. Please use value
> > returned by creating initial ordinal or add comment
> > you may do something const std::string initial_ordinal = ....
> > and substitute all "n". I am also ok with comment.
> 
> With tests, I think it's good to be as literal as possible when validating
> results and side effects.  I've added comments indicating that we expect the
> item_ordinal to be fixed up.

Acknowledged.

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:370:
EXPECT_EQ("item_name1x", GetSyncItem(kItemId1)->item_name);
On 2017/06/24 00:19:10, rcui wrote:
> On 2017/06/23 22:37:27, khmel wrote:
> > nit: It seems you frequently use this pattern. If you want you may add some
> > helper protected method (not forcing to do this):
> > 
> > void ValidateSyncItem(const std::string& id,
> >                       const std::string& expected_name,....) {
> > ...
> > }
> 
> I went ahead and tried it, and the issue with this approach is it's hard to
see
> which part of the test the failed expectation (EXPECT, ASSERT) belongs to,
> because all failures are reported as happening in ValidateSyncItem rather than
> its caller, and passing --gtest_throw_on_failure to the unit_test runner
doesn't
> work either.  Given it's a nice-to-have, I will go ahead without this.  Would
be
> happy to hear ways around this if you know of any.

Usually this is not a problem.
For example:
https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_u...

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:412:
ASSERT_TRUE(GetSyncItem(kItemId));
On 2017/06/24 00:19:11, rcui wrote:
> On 2017/06/23 22:37:27, khmel wrote:
> > nit: it would be nice to check that sync item was not modified after
applying
> > bad data.
> 
> Given the type of bad data we're using, the sync item would be modified, no?

Up to you, I just wanted  to see the use case when we apply bad data and sync
item is not modified (bad data was ignored). If really bad data is applied then
we might have a problem where that data is used. 
And if bad data is actually modifies the sync item we have to be sure that is
modified way we expect.
This is not a stopper for this cl.

[commit-bot: I haz the power, 2017-06-24 00:37:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498263582263410,
"parent_rev": "0d7335cd221d21f07fbc8dce72e437167e08ca7e", "commit_rev":
"a2a65e1fa1e5c88feb43b1d0b3264c3764d0481b"}

[commit-bot: I haz the power, 2017-06-24 00:37:23]

Description was changed from

==========
Tests that validate the merge and process functionality of 
AppListSyncableService with various kinds of good and bad data.

BUG=645137
==========

to

==========
Tests that validate the merge and process functionality of 
AppListSyncableService with various kinds of good and bad data.

BUG=645137

Review-Url: https://codereview.chromium.org/2952463002
Cr-Commit-Position: refs/heads/master@{#482092}
Committed:
https://chromium.googlesource.com/chromium/src/+/a2a65e1fa1e5c88feb43b1d0b326...
==========

[commit-bot: I haz the power, 2017-06-24 00:37:24]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a2a65e1fa1e5c88feb43b1d0b326...

[rcui, 2017-06-24 01:01:54]

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:370:
EXPECT_EQ("item_name1x", GetSyncItem(kItemId1)->item_name);
On 2017/06/24 00:30:16, khmel wrote:
> On 2017/06/24 00:19:10, rcui wrote:
> > On 2017/06/23 22:37:27, khmel wrote:
> > > nit: It seems you frequently use this pattern. If you want you may add
some
> > > helper protected method (not forcing to do this):
> > > 
> > > void ValidateSyncItem(const std::string& id,
> > >                       const std::string& expected_name,....) {
> > > ...
> > > }
> > 
> > I went ahead and tried it, and the issue with this approach is it's hard to
> see
> > which part of the test the failed expectation (EXPECT, ASSERT) belongs to,
> > because all failures are reported as happening in ValidateSyncItem rather
than
> > its caller, and passing --gtest_throw_on_failure to the unit_test runner
> doesn't
> > work either.  Given it's a nice-to-have, I will go ahead without this. 
Would
> be
> > happy to hear ways around this if you know of any.
> 
> Usually this is not a problem.
> For example:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_u...

It does seem like it'll make tests harder to maintain.  Digging around, it looks
like SCOPED_TRACE might be the solution:
https://g3doc.corp.google.com/third_party/gtest/g3doc/advanced.md#adding-trac....
 Will try it out and upload a separate CL.

https://codereview.chromium.org/2952463002/diff/60001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:412:
ASSERT_TRUE(GetSyncItem(kItemId));
On 2017/06/24 00:30:16, khmel wrote:
> On 2017/06/24 00:19:11, rcui wrote:
> > On 2017/06/23 22:37:27, khmel wrote:
> > > nit: it would be nice to check that sync item was not modified after
> applying
> > > bad data.
> > 
> > Given the type of bad data we're using, the sync item would be modified, no?
> 
> Up to you, I just wanted  to see the use case when we apply bad data and sync
> item is not modified (bad data was ignored). If really bad data is applied
then
> we might have a problem where that data is used. 
> And if bad data is actually modifies the sync item we have to be sure that is
> modified way we expect.
> This is not a stopper for this cl.

Acknowledged.