arc:Integration test for Arc package Chrome sync.

arc:Integration test for Arc package Chrome sync.

This Patch depends on:
https://codereview.chromium.org/2106663004

BUG=631253
TEST=Pass added integration tests.

Committed: https://crrev.com/f0d5430a0e13a019f7fbdbe69fbb63c92d9e719a
Cr-Commit-Position: refs/heads/master@{#409461}

[lgcheng, 2016-07-22 20:27:28]

Description was changed from

==========
arc:Integration test for Arc package Chrome sync.

BUG=28024050
TEST=Pass added integration tests.
==========

to

==========
arc:Integration test for Arc package Chrome sync.

BUG=28024050
TEST=Pass added integration tests.
==========

[lgcheng, 2016-07-22 20:27:28]

lgcheng@google.com changed reviewers:
+ elijahtaylor@chromium.org, lhchavez@chromium.org, pavely@chromium.org,
stevenjb@chromium.org

[lgcheng, 2016-07-22 20:28:03]

Hi Guys,

This is the integration test for Arc package chrome sync.

PTAL. Thanks!

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc:48: if
(is_sync_test_) {
Luis@ Are use satisfied with this test setup for ArcAppListPrefsFactory?

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:141:
pending_install_items_[package_name] = std::move(sync_item);
I change the order because InstallPackage is synchronized in mock arc
environment in test.

[lgcheng, 2016-07-22 21:02:28]

Description was changed from

==========
arc:Integration test for Arc package Chrome sync.

BUG=28024050
TEST=Pass added integration tests.
==========

to

==========
arc:Integration test for Arc package Chrome sync.

This Patch depends on:
https://codereview.chromium.org/2106663004

BUG=28024050
TEST=Pass added integration tests.
==========

[lgcheng, 2016-07-23 00:09:40]

lgcheng@google.com changed reviewers:
+ jhorwich@chromium.org

[lgcheng, 2016-07-23 00:11:26]

+ Josh
I think two_client_arc_package_sync_test.cc will be the file that you are
interested in as it contains the basic test cases.

[stevenjb, 2016-07-25 16:45:08]

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sync_arc_package_helper.h (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.h:31: void
ClearUp();
nit: CleanUp

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.h:57: bool
ArcPackageMatch(Profile* profile1, Profile* profile2);
ArcPackagesMatch

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.h:61: bool
ArcPackageDetailMatch(Profile* profile1, Profile* profile2);
ArcPackageDetailsMatch

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:21:
bool AllProfilesHaveSameArcPackageDetail() {
Details

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:71:
constexpr int kNumPackage = 5;
kNumPackages

[Josh Horwich, 2016-07-25 16:48:09]

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:78: char id =
'0';
Are you counting on pointer-to-this being a null-terminated C-style string? It
appears so later (&id gets passed to gaia_id::append as a const char* which is
expected to be a null-terminated C-style string) - if so, please address this.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:19: }
*/
Is this old code to be deleted?

[Luis Héctor Chávez, 2016-07-25 17:56:24]

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:211: if
(pref1_packages.size()!=pref2_packages.size())
can you run git cl format?

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sync_arc_package_helper.h (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.h:23: class
ArcBridgeService;
unused

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.h:33: void
InstallPackageWithIndex(Profile* profile, int id);
Can these ints be size_t?

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:72: for
(int i = 0; i < kNumPackage; ++i) {
Same here, s/int/size_t/

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc:48: if
(is_sync_test_) {
On 2016/07/22 20:28:02, lgcheng wrote:
> Luis@ Are use satisfied with this test setup for ArcAppListPrefsFactory?

Almost :) L50 will leak, you need to hold a unique_ptr somewhere. Or make it a
keyedservicefactory so it will have a strong affinity with the profile.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.h (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.h:23: static void
SetFactoryForSynctTest();
nit: SetFactoryForSyncTest

[lgcheng, 2016-07-25 20:45:32]

Issue fixed. PTAL

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:78: char id =
'0';
On 2016/07/25 16:48:08, Josh Horwich wrote:
> Are you counting on pointer-to-this being a null-terminated C-style string? It
> appears so later (&id gets passed to gaia_id::append as a const char* which is
> expected to be a null-terminated C-style string) - if so, please address this.

It was a poor and confusion implementation to use char* for id. I change it to
size_t now. Thanks for point this out.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:211: if
(pref1_packages.size()!=pref2_packages.size())
On 2016/07/25 17:56:24, Luis Héctor Chávez wrote:
> can you run git cl format?

Done.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/sync_arc_package_helper.h (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.h:23: class
ArcBridgeService;
On 2016/07/25 17:56:24, Luis Héctor Chávez wrote:
> unused

Done.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.h:31: void
ClearUp();
On 2016/07/25 16:45:08, stevenjb wrote:
> nit: CleanUp

Done.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.h:33: void
InstallPackageWithIndex(Profile* profile, int id);
On 2016/07/25 17:56:24, Luis Héctor Chávez wrote:
> Can these ints be size_t?

Done.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.h:57: bool
ArcPackageMatch(Profile* profile1, Profile* profile2);
On 2016/07/25 16:45:08, stevenjb wrote:
> ArcPackagesMatch
> 

Done.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/sync_arc_package_helper.h:61: bool
ArcPackageDetailMatch(Profile* profile1, Profile* profile2);
On 2016/07/25 16:45:08, stevenjb wrote:
> ArcPackageDetailsMatch

Done.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:19: }
*/
On 2016/07/25 16:48:08, Josh Horwich wrote:
> Is this old code to be deleted?

This is a preserved method. I plan to use this for package update logic. But
this patch only handles install/uninstall. I can remove it if needed in this
patch.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:21:
bool AllProfilesHaveSameArcPackageDetail() {
On 2016/07/25 16:45:08, stevenjb wrote:
> Details

Done.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:71:
constexpr int kNumPackage = 5;
On 2016/07/25 16:45:08, stevenjb wrote:
> kNumPackages

Done.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:72: for
(int i = 0; i < kNumPackage; ++i) {
On 2016/07/25 17:56:24, Luis Héctor Chávez wrote:
> Same here, s/int/size_t/

Done.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.cc:48: if
(is_sync_test_) {
On 2016/07/25 17:56:24, Luis Héctor Chávez wrote:
> On 2016/07/22 20:28:02, lgcheng wrote:
> > Luis@ Are use satisfied with this test setup for ArcAppListPrefsFactory?
> 
> Almost :) L50 will leak, you need to hold a unique_ptr somewhere. Or make it a
> keyedservicefactory so it will have a strong affinity with the profile.

unique_ptr holder added.

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.h (right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.h:23: static void
SetFactoryForSynctTest();
On 2016/07/25 17:56:24, Luis Héctor Chávez wrote:
> nit: SetFactoryForSyncTest

Done.

[Luis Héctor Chávez, 2016-07-25 20:57:25]

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:19: }
*/
On 2016/07/25 20:45:32, lgcheng wrote:
> On 2016/07/25 16:48:08, Josh Horwich wrote:
> > Is this old code to be deleted?
> 
> This is a preserved method. I plan to use this for package update logic. But
> this patch only handles install/uninstall. I can remove it if needed in this
> patch.

Yes please. There should be zero commented-out code in the repository. (except
maybe usage examples).

https://codereview.chromium.org/2174753004/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc:63:
const size_t kNumPackages = 5;
nit: constexpr

https://codereview.chromium.org/2174753004/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2174753004/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:162:
FakeAppInstance* fake_app_instance = new FakeAppInstance(arc_app_list_prefs);
Oops, I didn't notice this before. Can you store this pointer somewhere in a
std::unique_ptr? Same if you see another naked new.

[lgcheng, 2016-07-25 21:33:50]

Patchset #3 (id:40001) has been deleted

[lgcheng, 2016-07-25 21:37:28]

PTAL this update.

Thanks!

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/1/chrome/browser/sync/test/in...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:19: }
*/
On 2016/07/25 20:57:24, Luis Héctor Chávez wrote:
> On 2016/07/25 20:45:32, lgcheng wrote:
> > On 2016/07/25 16:48:08, Josh Horwich wrote:
> > > Is this old code to be deleted?
> > 
> > This is a preserved method. I plan to use this for package update logic. But
> > this patch only handles install/uninstall. I can remove it if needed in this
> > patch.
> 
> Yes please. There should be zero commented-out code in the repository. (except
> maybe usage examples).

Done.

https://codereview.chromium.org/2174753004/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc:63:
const size_t kNumPackages = 5;
On 2016/07/25 20:57:24, Luis Héctor Chávez wrote:
> nit: constexpr

Done.

https://codereview.chromium.org/2174753004/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2174753004/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:162:
FakeAppInstance* fake_app_instance = new FakeAppInstance(arc_app_list_prefs);
On 2016/07/25 20:57:24, Luis Héctor Chávez wrote:
> Oops, I didn't notice this before. Can you store this pointer somewhere in a
> std::unique_ptr? Same if you see another naked new.

Ah sorry for this. I thought these leaks were not important in browser_tests as
the life circle of all browser_tests is short... Now I have unique_ptr wrappers
for the objects.

[Josh Horwich, 2016-07-25 22:14:50]

https://codereview.chromium.org/2174753004/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2174753004/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:41: std::string
gaia_id = "1234567890" + base::IntToString(id);
base::SizeTToString

[lgcheng, 2016-07-25 22:26:39]

https://codereview.chromium.org/2174753004/diff/60001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2174753004/diff/60001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:41: std::string
gaia_id = "1234567890" + base::IntToString(id);
On 2016/07/25 22:14:50, Josh Horwich wrote:
> base::SizeTToString

Done.

[lgcheng, 2016-07-25 23:04:44]

Description was changed from

==========
arc:Integration test for Arc package Chrome sync.

This Patch depends on:
https://codereview.chromium.org/2106663004

BUG=28024050
TEST=Pass added integration tests.
==========

to

==========
arc:Integration test for Arc package Chrome sync.

This Patch depends on:
https://codereview.chromium.org/2106663004

BUG=631253
TEST=Pass added integration tests.
==========

[Luis Héctor Chávez, 2016-07-26 17:30:53]

lgtm with nits

https://codereview.chromium.org/2174753004/diff/100001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2174753004/diff/100001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:107:
ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile);
nit: const ArcAppListPrefs*

https://codereview.chromium.org/2174753004/diff/100001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:149: // Having
the user-to-profile mapping ready to avoid using the real profile
nit: s/Having/Have/.

[lgcheng, 2016-07-26 17:46:13]

Thanks Luis for review!

Steven, PTAL
chrome/browser/chromeos/profiles/profile_helper.h
chrome/browser/ui/app_list/*

pavely@, PTAL
chrome/browser/sync/test/*

Thanks!

https://codereview.chromium.org/2174753004/diff/100001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_arc_package_helper.cc (right):

https://codereview.chromium.org/2174753004/diff/100001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:107:
ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile);
On 2016/07/26 17:30:52, Luis Héctor Chávez wrote:
> nit: const ArcAppListPrefs*

Done.

https://codereview.chromium.org/2174753004/diff/100001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_arc_package_helper.cc:149: // Having
the user-to-profile mapping ready to avoid using the real profile
On 2016/07/26 17:30:52, Luis Héctor Chávez wrote:
> nit: s/Having/Have/.

Done.

[stevenjb, 2016-07-27 16:45:46]

lgtm

[lgcheng, 2016-07-27 17:10:29]

On 2016/07/27 16:45:46, stevenjb wrote:
> lgtm

Thank Steven for review this!

Pavel, I know you are ooo until this Thursday. Would you review
chrome/browser/sync/test/* when you are back?

Thanks!

[lgcheng, 2016-07-29 21:26:59]

Hi Pavel,

Could you revew
chrome/browser/sync/test/*
when you have a moment? Or if you don't have the slot, would you suggest anyone
who can review/approve the cl? It is urgent as the bug is targeted to M53.

Thanks!

[pavely, 2016-07-29 23:01:04]

chrome/browser/sync/test/* lgtm % couple of comments.

https://codereview.chromium.org/2174753004/diff/140001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/140001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc:56:
CleanUp();
ASSERT_TRUE returns early. To ensure cleanup code is executed put it in
TearDown().

https://codereview.chromium.org/2174753004/diff/140001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/140001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:24: :
SyncTest(TWO_CLIENT_LEGACY), sync_helper_(nullptr) {
Why do you pass TWO_CLIENT_LEGACY? Could you try running with TWO_CLIENT?

https://codereview.chromium.org/2174753004/diff/140001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:55:
CleanUp();
ASSERT_TRUE returns early. To ensure cleanup code is executed put it in
TearDown().

[lgcheng, 2016-07-29 23:53:21]

Thank pavel for review!

Issue addressed.

https://codereview.chromium.org/2174753004/diff/140001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/140001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc:56:
CleanUp();
On 2016/07/29 23:01:03, pavely wrote:
> ASSERT_TRUE returns early. To ensure cleanup code is executed put it in
> TearDown().

I put clearup code in TearDownOnMainThread(). Thanks for pointing out.

https://codereview.chromium.org/2174753004/diff/140001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/140001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:24: :
SyncTest(TWO_CLIENT_LEGACY), sync_helper_(nullptr) {
On 2016/07/29 23:01:03, pavely wrote:
> Why do you pass TWO_CLIENT_LEGACY? Could you try running with TWO_CLIENT?
There are three main reasons for this:

1.TWO_CLIENT_LEGACY(local python server) gives me more logs while I create these
tests.
2.AwaitQuiescence() can only be used in local mock server. I need create my own
api to wait until sync finished if I use real server. But that cost extra time
while the issue is targeted to M53... Sorry 
3.It seems real server has not been ready for the new data type yet.(alpha,
integration test, prod ...) In-ho is working on server side to apply the new
datatype.

https://codereview.chromium.org/2174753004/diff/140001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:55:
CleanUp();
On 2016/07/29 23:01:03, pavely wrote:
> ASSERT_TRUE returns early. To ensure cleanup code is executed put it in
> TearDown().
I put clearup code in TearDownOnMainThread(). Thanks for pointing out and making
the code neat and correct!

[Luis Héctor Chávez, 2016-08-01 18:03:40]

https://codereview.chromium.org/2174753004/diff/160001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/160001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc:42:
sync_helper_->CleanUp();
For symmetry, can |sync_helper_| be initialized in SetUpOnMainThread()?

Also, maybe |sync_helper_ = nullptr;|?

https://codereview.chromium.org/2174753004/diff/160001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/160001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:41:
sync_helper_->CleanUp();
same here.

[lgcheng, 2016-08-01 18:14:18]

https://codereview.chromium.org/2174753004/diff/160001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/160001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/single_client_arc_package_sync_test.cc:42:
sync_helper_->CleanUp();
On 2016/08/01 18:03:40, Luis Héctor Chávez wrote:
> For symmetry, can |sync_helper_| be initialized in SetUpOnMainThread()?
> 
> Also, maybe |sync_helper_ = nullptr;|?

Done.

https://codereview.chromium.org/2174753004/diff/160001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc
(right):

https://codereview.chromium.org/2174753004/diff/160001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/two_client_arc_package_sync_test.cc:41:
sync_helper_->CleanUp();
On 2016/08/01 18:03:40, Luis Héctor Chávez wrote:
> same here.

Done.

[lgcheng, 2016-08-03 04:09:35]

The CQ bit was checked by lgcheng@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 04:09:45]

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, 2016-08-03 05:13:30]

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

[commit-bot: I haz the power, 2016-08-03 05:13:31]

Dry run: This issue passed the CQ dry run.

[lgcheng, 2016-08-03 05:17:36]

The CQ bit was checked by lgcheng@google.com

[lgcheng, 2016-08-03 05:17:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, stevenjb@chromium.org, pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2174753004/#ps220001
(title: "Modify target")

[commit-bot: I haz the power, 2016-08-03 05:17:45]

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, 2016-08-03 05:21:26]

Description was changed from

==========
arc:Integration test for Arc package Chrome sync.

This Patch depends on:
https://codereview.chromium.org/2106663004

BUG=631253
TEST=Pass added integration tests.
==========

to

==========
arc:Integration test for Arc package Chrome sync.

This Patch depends on:
https://codereview.chromium.org/2106663004

BUG=631253
TEST=Pass added integration tests.
==========

[commit-bot: I haz the power, 2016-08-03 05:21:27]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-08-03 05:23:26]

Description was changed from

==========
arc:Integration test for Arc package Chrome sync.

This Patch depends on:
https://codereview.chromium.org/2106663004

BUG=631253
TEST=Pass added integration tests.
==========

to

==========
arc:Integration test for Arc package Chrome sync.

This Patch depends on:
https://codereview.chromium.org/2106663004

BUG=631253
TEST=Pass added integration tests.

Committed: https://crrev.com/f0d5430a0e13a019f7fbdbe69fbb63c92d9e719a
Cr-Commit-Position: refs/heads/master@{#409461}
==========

[commit-bot: I haz the power, 2016-08-03 05:23:27]

Patchset 11 (id:??) landed as
https://crrev.com/f0d5430a0e13a019f7fbdbe69fbb63c92d9e719a
Cr-Commit-Position: refs/heads/master@{#409461}