Fix ArcAppModelBuilderTest

Fix ArcAppModelBuilderTest

Several tests in this suite were flaky. This change:

* Sets up a real I/O thread for the ExtensionsService and
  sets the profile mapping in case the garbage collector
  kicks in.
* Does a better job of waiting for the icons to be loaded.
* Uniformly disables safe icon decoding since it introduces
  extra thread hops that are difficult to synchronize for.

BUG=627853
BUG=624446
TEST=./unit_tests --gtest-filter=ArcAppModelBuilderTest*
TEST=for each test:
     ./unit_tests --gtest-filter=${t} --single-process-tests

Committed: https://crrev.com/4303e815b33c5d970e210ca9edd2b6bafedc8ad3
Cr-Commit-Position: refs/heads/master@{#405643}

[Luis Héctor Chávez, 2016-07-13 16:25:53]

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

[Luis Héctor Chávez, 2016-07-13 16:26:04]

lhchavez@chromium.org changed reviewers:
+ jennyz@chromium.org

[Luis Héctor Chávez, 2016-07-13 16:26:05]

PTAL

[commit-bot: I haz the power, 2016-07-13 16:26:38]

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-07-13 17:05:12]

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

[commit-bot: I haz the power, 2016-07-13 17:05:13]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-07-14 22:15:08]

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

[commit-bot: I haz the power, 2016-07-14 22:15:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Luis Héctor Chávez, 2016-07-14 22:23:05]

Description was changed from

==========
Fix ArcAppModelBuilderTest.RemoveAppCleanUpFolder

This test was flaky since it injected events out of order into the
various RunLoops. This change ensures that the UI RunLoop is empty
before sending an image to the decoding thread.

BUG=627853
TEST=./unit_tests --gtest-filter=ArcAppModelBuilderTest*
TEST=./unit_tests \
     --gtest-filter=ArcAppModelBuilderTest.RemoveAppCleanUpFolder
==========

to

==========
Fix ArcAppModelBuilderTest

Several tests in this suite were flaky. This change:

* Sets up a real I/O thread for the ExtensionsService and
  sets the profile mapping in case the garbage collector
  kicks in.
* Does a better job of waiting for the icons to be loaded.
* Uniformly disables safe icon decoding since it introduces
  extra thread hops that are difficult to synchronize for.

BUG=627853
BUG=624446
TEST=./unit_tests --gtest-filter=ArcAppModelBuilderTest*
TEST=for each test:
     ./unit_tests --gtest-filter=${t} --single-process-tests
==========

[Luis Héctor Chávez, 2016-07-14 22:23:05]

lhchavez@chromium.org changed reviewers:
+ xiyuan@chromium.org
- jennyz@chromium.org

[Luis Héctor Chávez, 2016-07-14 22:41:29]

xiyuan PTAL *

This should make tests less flaky

[Luis Héctor Chávez, 2016-07-14 22:44:32]

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.h (right):

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.h:8: #include <map>
`git cl lint` asked me to do this, as well as L236.

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:864:
TEST_F(ArcAppModelBuilderTest, IconLoader) {
This test is unfortunately still flaky (5-10%) even after doing all this. Should
I disable it?

[xiyuan, 2016-07-14 22:59:33]

lgtm

Just nits.

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.h (right):

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.h:8: #include <map>
On 2016/07/14 22:44:32, Luis Héctor Chávez wrote:
> `git cl lint` asked me to do this, as well as L236.

Acknowledged.

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:816: } while
(base::PathExists(app_path));
Could this be WaitForIconReady ?

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:864:
TEST_F(ArcAppModelBuilderTest, IconLoader) {
On 2016/07/14 22:44:32, Luis Héctor Chávez wrote:
> This test is unfortunately still flaky (5-10%) even after doing all this.
Should
> I disable it?

Yes, we should disable it until resolved so that sheriffs have an easier life.

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:890: ArcAppItem* app_item =
FindArcItem(ArcAppTest::GetAppId(app));
nit: Isn't ArcAppTest::GetAppId(app) here the |app_id| in line 866?

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:898: WaitForIconReady(prefs,
ArcAppTest::GetAppId(app), scale_factor);
nit: ArcAppTest::GetAppId(app) -> app_id ?

[commit-bot: I haz the power, 2016-07-14 22:59:51]

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

[commit-bot: I haz the power, 2016-07-14 22:59:51]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-07-14 23:25:45]

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_unittest.cc (right):

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:816: } while
(base::PathExists(app_path));
On 2016/07/14 22:59:32, xiyuan wrote:
> Could this be WaitForIconReady ?

No, it needs the opposite condition. I'll add a comment.

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:890: ArcAppItem* app_item =
FindArcItem(ArcAppTest::GetAppId(app));
On 2016/07/14 22:59:32, xiyuan wrote:
> nit: Isn't ArcAppTest::GetAppId(app) here the |app_id| in line 866?

Good catch, done.

https://codereview.chromium.org/2150583002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_unittest.cc:898: WaitForIconReady(prefs,
ArcAppTest::GetAppId(app), scale_factor);
On 2016/07/14 22:59:32, xiyuan wrote:
> nit: ArcAppTest::GetAppId(app) -> app_id ?

Done.

[Luis Héctor Chávez, 2016-07-14 23:25:53]

The CQ bit was checked by lhchavez@chromium.org

[Luis Héctor Chávez, 2016-07-14 23:25:54]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2150583002/#ps40001
(title: "Addressed feedback")

[commit-bot: I haz the power, 2016-07-14 23:26:21]

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-07-15 00:04:20]

Description was changed from

==========
Fix ArcAppModelBuilderTest

Several tests in this suite were flaky. This change:

* Sets up a real I/O thread for the ExtensionsService and
  sets the profile mapping in case the garbage collector
  kicks in.
* Does a better job of waiting for the icons to be loaded.
* Uniformly disables safe icon decoding since it introduces
  extra thread hops that are difficult to synchronize for.

BUG=627853
BUG=624446
TEST=./unit_tests --gtest-filter=ArcAppModelBuilderTest*
TEST=for each test:
     ./unit_tests --gtest-filter=${t} --single-process-tests
==========

to

==========
Fix ArcAppModelBuilderTest

Several tests in this suite were flaky. This change:

* Sets up a real I/O thread for the ExtensionsService and
  sets the profile mapping in case the garbage collector
  kicks in.
* Does a better job of waiting for the icons to be loaded.
* Uniformly disables safe icon decoding since it introduces
  extra thread hops that are difficult to synchronize for.

BUG=627853
BUG=624446
TEST=./unit_tests --gtest-filter=ArcAppModelBuilderTest*
TEST=for each test:
     ./unit_tests --gtest-filter=${t} --single-process-tests
==========

[commit-bot: I haz the power, 2016-07-15 00:04:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-15 00:04:43]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 00:05:41]

Description was changed from

==========
Fix ArcAppModelBuilderTest

Several tests in this suite were flaky. This change:

* Sets up a real I/O thread for the ExtensionsService and
  sets the profile mapping in case the garbage collector
  kicks in.
* Does a better job of waiting for the icons to be loaded.
* Uniformly disables safe icon decoding since it introduces
  extra thread hops that are difficult to synchronize for.

BUG=627853
BUG=624446
TEST=./unit_tests --gtest-filter=ArcAppModelBuilderTest*
TEST=for each test:
     ./unit_tests --gtest-filter=${t} --single-process-tests
==========

to

==========
Fix ArcAppModelBuilderTest

Several tests in this suite were flaky. This change:

* Sets up a real I/O thread for the ExtensionsService and
  sets the profile mapping in case the garbage collector
  kicks in.
* Does a better job of waiting for the icons to be loaded.
* Uniformly disables safe icon decoding since it introduces
  extra thread hops that are difficult to synchronize for.

BUG=627853
BUG=624446
TEST=./unit_tests --gtest-filter=ArcAppModelBuilderTest*
TEST=for each test:
     ./unit_tests --gtest-filter=${t} --single-process-tests

Committed: https://crrev.com/4303e815b33c5d970e210ca9edd2b6bafedc8ad3
Cr-Commit-Position: refs/heads/master@{#405643}
==========

[commit-bot: I haz the power, 2016-07-15 00:05:42]

Patchset 3 (id:??) landed as
https://crrev.com/4303e815b33c5d970e210ca9edd2b6bafedc8ad3
Cr-Commit-Position: refs/heads/master@{#405643}