Set enable_app_list=1 now only on ChromeOS.

Set enable_app_list=1 now only on ChromeOS.

Turn down app_list_unittests on Mac. Moves app_list_unittests and
app_list_presenter_unittests to the same place as ash_unittests on other
non-ChromeOS bots.

BUG=600915
Committed: https://crrev.com/87ca2d6836730fb7cacf7c76e33e100df599b754
Cr-Commit-Position: refs/heads/master@{#394856}

[tapted, 2016-05-18 07:05:03]

Patchset #5 (id:80001) has been deleted

[tapted, 2016-05-18 07:05:11]

Patchset #4 (id:60001) has been deleted

[tapted, 2016-05-18 09:42:19]

Patchset #3 (id:40001) has been deleted

[tapted, 2016-05-18 09:42:28]

Patchset #3 (id:100001) has been deleted

[tapted, 2016-05-18 09:42:38]

Patchset #3 (id:120001) has been deleted

[tapted, 2016-05-18 11:54:51]

Patchset #5 (id:180001) has been deleted

[tapted, 2016-05-18 12:24:17]

Description was changed from

==========
Set enable_app_list=1 now only on ChromeOS.

Turn down app_list_unittests on non-ChromeOS bots.

BUG=600915
==========

to

==========
Set enable_app_list=1 now only on ChromeOS.

Turn down app_list_unittests and app_list_presenter_unittests on
non-ChromeOS bots.

BUG=600915
==========

[tapted, 2016-05-18 12:25:30]

tapted@chromium.org changed reviewers:
+ dpranke@chromium.org, sky@chromium.org

[tapted, 2016-05-18 12:45:24]

Hi Dirk and sky, please take a look

Dirk mainly for src/build
sky for the ash_test_resources change, and testing/buildbot with a question:

the ash_test_resources change was required because ash_unittests still runs on
the Windows bots. app_list is rather entwined with ash, and enable_app_list
(upper or lowercase) doesn't appear anywhere else under src/ash - so I think
that bit is right.

The bit I'm *not* so sure about it whether app_list_unittests should be turned
down on the Windows/Linux bots at all. The reasons to keep it there are probably
the same reasons why we still run ash_unittests. Do we want to keep
app_list_[presenter]_unittests on Windows/Linux?

(I think I would also have to move app_list_unittests to sit next to
ash_unittests in all.gyp and src/BUILD.gn -- 3 places over both, all behind
"use_aura").

[Dirk Pranke, 2016-05-18 15:49:21]

lgtm.

https://codereview.chromium.org/1934213002/diff/200001/testing/buildbot/chrom...
File testing/buildbot/chromium_trybot.json (right):

https://codereview.chromium.org/1934213002/diff/200001/testing/buildbot/chrom...
testing/buildbot/chromium_trybot.json:23: ],
I'm guessing you added these lines because they're used elsewhere in this file,
right?

This file isn't used, so you don't need these entries.

I'm actually going to post a separate CL to delete this file altogether.

[sky, 2016-05-18 16:44:56]

On 2016/05/18 12:45:24, tapted wrote:
> Hi Dirk and sky, please take a look
> 
> Dirk mainly for src/build
> sky for the ash_test_resources change, and testing/buildbot with a question:
> 
> the ash_test_resources change was required because ash_unittests still runs on
> the Windows bots. app_list is rather entwined with ash, and enable_app_list
> (upper or lowercase) doesn't appear anywhere else under src/ash - so I think
> that bit is right.
> 
> The bit I'm *not* so sure about it whether app_list_unittests should be turned
> down on the Windows/Linux bots at all. The reasons to keep it there are
probably
> the same reasons why we still run ash_unittests. Do we want to keep
> app_list_[presenter]_unittests on Windows/Linux?
> 
> (I think I would also have to move app_list_unittests to sit next to
> ash_unittests in all.gyp and src/BUILD.gn -- 3 places over both, all behind
> "use_aura").

If we keep app_list_unittests for windows/linux, what exactly is it testing? I
tend to think if we're running ash_unittests on windows/linux, and ash_unittests
uses the app_list, then we should also run app_list_unittests. But I want to
makes ure I'm not missing something.

[tapted, 2016-05-18 21:54:16]

Patchset #6 (id:220001) has been deleted

[tapted, 2016-05-18 21:55:53]

Description was changed from

==========
Set enable_app_list=1 now only on ChromeOS.

Turn down app_list_unittests and app_list_presenter_unittests on
non-ChromeOS bots.

BUG=600915
==========

to

==========
Set enable_app_list=1 now only on ChromeOS.

Turn down app_list_unittests on Mac. Moves app_list_unittests and
app_list_presenter_unittests to the same place as ash_unittests on other
non-ChromeOS bots.

BUG=600915
==========

[tapted, 2016-05-18 22:01:37]

On 2016/05/18 16:44:56, sky wrote:
> > The bit I'm *not* so sure about it whether app_list_unittests should be
turned
> > down on the Windows/Linux bots at all. The reasons to keep it there are
> probably
> > the same reasons why we still run ash_unittests. Do we want to keep
> > app_list_[presenter]_unittests on Windows/Linux?
> > 
> > (I think I would also have to move app_list_unittests to sit next to
> > ash_unittests in all.gyp and src/BUILD.gn -- 3 places over both, all behind
> > "use_aura").

(done)

> 
> If we keep app_list_unittests for windows/linux, what exactly is it testing? I
> tend to think if we're running ash_unittests on windows/linux, and
ash_unittests
> uses the app_list, then we should also run app_list_unittests. But I want to
> makes ure I'm not missing something.

Yup - that's my reasoning too. app_list_unittests is like an elaborate (set of)
views_unittests -- it has its own models and tests interactions with its various
View subclasses mostly. It depends mostly on ui/views. All the ash and chrome
dependencies are delegated out, so I guess it sits between ash and views in the
layer cake.

There are other tests in the chrome unit_tests/browser_tests targets -- these
will now just run on ChromeOS after this.

I've tweaked the CL to keep app_list_unittests in the same places as
ash_unittests so you can try it on. Bots will hopefully be happy too....  but it
might take a a while to find out.

https://codereview.chromium.org/1934213002/diff/200001/testing/buildbot/chrom...
File testing/buildbot/chromium_trybot.json (right):

https://codereview.chromium.org/1934213002/diff/200001/testing/buildbot/chrom...
testing/buildbot/chromium_trybot.json:23: ],
On 2016/05/18 15:49:21, Dirk Pranke wrote:
> I'm guessing you added these lines because they're used elsewhere in this
file,
> right?
> 
> This file isn't used, so you don't need these entries.
> 
> I'm actually going to post a separate CL to delete this file altogether.

Done.

[tapted, 2016-05-19 11:49:12]

sky, PTAL

On 2016/05/18 22:01:37, tapted wrote:
> I've tweaked the CL to keep app_list_unittests in the same places as
> ash_unittests so you can try it on. Bots will hopefully be happy too....  but
it
> might take a a while to find out.

And they're happy \o/. ui_test_pak needed a slight tweak - I this is the right
approach. Now ash_ and app_list_ unittests are together tied to use_aura. (and
app_list_resources.grd picks up a use_aura to include in ui_test_pak to match).

[sky, 2016-05-19 16:34:32]

LGTM

[tapted, 2016-05-19 20:42:27]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-05-19 20:42:28]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1934213002/#ps260001
(title: "Update ui_test_pak - should fix tests")

[commit-bot: I haz the power, 2016-05-19 20:43:02]

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

[commit-bot: I haz the power, 2016-05-19 20:49:04]

Description was changed from

==========
Set enable_app_list=1 now only on ChromeOS.

Turn down app_list_unittests on Mac. Moves app_list_unittests and
app_list_presenter_unittests to the same place as ash_unittests on other
non-ChromeOS bots.

BUG=600915
==========

to

==========
Set enable_app_list=1 now only on ChromeOS.

Turn down app_list_unittests on Mac. Moves app_list_unittests and
app_list_presenter_unittests to the same place as ash_unittests on other
non-ChromeOS bots.

BUG=600915
==========

[commit-bot: I haz the power, 2016-05-19 20:49:05]

Committed patchset #7 (id:260001)

[commit-bot: I haz the power, 2016-05-19 20:50:20]

Description was changed from

==========
Set enable_app_list=1 now only on ChromeOS.

Turn down app_list_unittests on Mac. Moves app_list_unittests and
app_list_presenter_unittests to the same place as ash_unittests on other
non-ChromeOS bots.

BUG=600915
==========

to

==========
Set enable_app_list=1 now only on ChromeOS.

Turn down app_list_unittests on Mac. Moves app_list_unittests and
app_list_presenter_unittests to the same place as ash_unittests on other
non-ChromeOS bots.

BUG=600915

Committed: https://crrev.com/87ca2d6836730fb7cacf7c76e33e100df599b754
Cr-Commit-Position: refs/heads/master@{#394856}
==========

[commit-bot: I haz the power, 2016-05-19 20:50:21]

Patchset 7 (id:??) landed as
https://crrev.com/87ca2d6836730fb7cacf7c76e33e100df599b754
Cr-Commit-Position: refs/heads/master@{#394856}

[hans, 2016-05-20 17:10:34]

hans@chromium.org changed reviewers:
+ hans@chromium.org

[hans, 2016-05-20 17:10:35]

This broke "isolate_tests" on e.g. this bot:
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/5287/...

It seems app_list_presenter_unittests_run is no longer built as part of "all".

One way to reproduce (on Linux):

$ GYP_DEFINES='test_isolation_mode=prepare' build/gyp_chromium
$ ninja -C out/Release -n | grep app_list_presenter_unittests_run
(crickets)


I'm not exactly what part of this change made it break though.

[hans, 2016-05-20 17:12:59]

On 2016/05/20 17:10:35, hans wrote:
> This broke "isolate_tests" on e.g. this bot:
>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/5287/...
> 
> It seems app_list_presenter_unittests_run is no longer built as part of "all".
> 
> One way to reproduce (on Linux):
> 
> $ GYP_DEFINES='test_isolation_mode=prepare' build/gyp_chromium
> $ ninja -C out/Release -n | grep app_list_presenter_unittests_run
> (crickets)
> 
> 
> I'm not exactly what part of this change made it break though.

D'oh, now I see! It's the condition in gn_migration.gypi that needs updating:
https://code.google.com/p/chromium/codesearch#chromium/src/build/gn_migration...

I'll prepare a patch.