Add test for system tray network menu icon for extension-controlled networks

Add test for system tray network menu icon for extension-controlled networks

Eventually extension-controlled network information will come out of a mojo
service. This provides test coverage to make sure the icon shows up in the
system tray menu.

BUG=651157
TEST=chrome browser_tests

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

[James Cook, 2016-12-08 00:30:40]

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

[James Cook, 2016-12-08 00:31:04]

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

[James Cook, 2016-12-08 00:31:05]

Steven, please take a look.

[commit-bot: I haz the power, 2016-12-08 00:31:25]

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-12-08 00:58:19]

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

[commit-bot: I haz the power, 2016-12-08 00:58:20]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-12-08 17:07:11]

Hmm. I'm actually kind of not excited about this. Even though the code is
already testing the message center implementation (which it really shouldn't),
I'm not thrilled about adding more UI dependencies (and specifically, ash
dependencies to src/chrome code) to an 'apitest'.

What is the long term plan for where ash + chrome integration tests should live?

Regardless, this seems like it should be in a browser test for the delegate, not
the apitest.

[James Cook, 2016-12-08 17:20:29]

On 2016/12/08 17:07:11, stevenjb wrote:
> Hmm. I'm actually kind of not excited about this. Even though the code is
> already testing the message center implementation (which it really shouldn't),
> I'm not thrilled about adding more UI dependencies (and specifically, ash
> dependencies to src/chrome code) to an 'apitest'.
> 
> What is the long term plan for where ash + chrome integration tests should
live?
> 
> Regardless, this seems like it should be in a browser test for the delegate,
not
> the apitest.

I think ash/chrome integration tests will be browser tests in chrome.

How about I make a new test for the delegate impl in c/b/ui/ash?

[stevenjb, 2016-12-08 17:22:34]

I think that would be better, thanks!


On Thu, Dec 8, 2016 at 10:20 AM, <jamescook@chromium.org> wrote:

> On 2016/12/08 17:07:11, stevenjb wrote:
> > Hmm. I'm actually kind of not excited about this. Even though the code is
> > already testing the message center implementation (which it really
> shouldn't),
> > I'm not thrilled about adding more UI dependencies (and specifically, ash
> > dependencies to src/chrome code) to an 'apitest'.
> >
> > What is the long term plan for where ash + chrome integration tests
> should
> live?
> >
> > Regardless, this seems like it should be in a browser test for the
> delegate,
> not
> > the apitest.
>
> I think ash/chrome integration tests will be browser tests in chrome.
>
> How about I make a new test for the delegate impl in c/b/ui/ash?
>
>
> https://codereview.chromium.org/2558083003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[James Cook, 2016-12-08 17:37:29]

On 2016/12/08 17:22:34, stevenjb wrote:
> I think that would be better, thanks!
> 
> 
> On Thu, Dec 8, 2016 at 10:20 AM, <mailto:jamescook@chromium.org> wrote:
> 
> > On 2016/12/08 17:07:11, stevenjb wrote:
> > > Hmm. I'm actually kind of not excited about this. Even though the code is
> > > already testing the message center implementation (which it really
> > shouldn't),
> > > I'm not thrilled about adding more UI dependencies (and specifically, ash
> > > dependencies to src/chrome code) to an 'apitest'.
> > >
> > > What is the long term plan for where ash + chrome integration tests
> > should
> > live?
> > >
> > > Regardless, this seems like it should be in a browser test for the
> > delegate,
> > not
> > > the apitest.
> >
> > I think ash/chrome integration tests will be browser tests in chrome.
> >
> > How about I make a new test for the delegate impl in c/b/ui/ash?
> >
> >
> > https://codereview.chromium.org/2558083003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Are you OK with the test of the delegate still deriving from ExtensionApiTest
and using the same test extension? I need to load the test extension and run it
to get it to register the SSID. There's probably a way to do this with an
ExtensionTest, but I don't know how off the top of my head and I would have to
create a new test extension that duplicates some of the existing one.

[stevenjb, 2016-12-08 17:52:33]

I haven't done this in a while, but I think what you want
is ExtensionBrowserTest.


On Thu, Dec 8, 2016 at 10:37 AM, <jamescook@chromium.org> wrote:

> On 2016/12/08 17:22:34, stevenjb wrote:
> > I think that would be better, thanks!
> >
> >
> > On Thu, Dec 8, 2016 at 10:20 AM, <mailto:jamescook@chromium.org> wrote:
> >
> > > On 2016/12/08 17:07:11, stevenjb wrote:
> > > > Hmm. I'm actually kind of not excited about this. Even though the
> code is
> > > > already testing the message center implementation (which it really
> > > shouldn't),
> > > > I'm not thrilled about adding more UI dependencies (and
> specifically, ash
> > > > dependencies to src/chrome code) to an 'apitest'.
> > > >
> > > > What is the long term plan for where ash + chrome integration tests
> > > should
> > > live?
> > > >
> > > > Regardless, this seems like it should be in a browser test for the
> > > delegate,
> > > not
> > > > the apitest.
> > >
> > > I think ash/chrome integration tests will be browser tests in chrome.
> > >
> > > How about I make a new test for the delegate impl in c/b/ui/ash?
> > >
> > >
> > > https://codereview.chromium.org/2558083003/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> Are you OK with the test of the delegate still deriving from
> ExtensionApiTest
> and using the same test extension? I need to load the test extension and
> run it
> to get it to register the SSID. There's probably a way to do this with an
> ExtensionTest, but I don't know how off the top of my head and I would
> have to
> create a new test extension that duplicates some of the existing one.
>
>
> https://codereview.chromium.org/2558083003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[James Cook, 2016-12-08 17:56:05]

Sorry, ExtensionBrowserTest is what I meant. That might work, but I'll need a
duplicate test extension that doesn't have all the extension API test stuff. Is
that OK, or would you prefer me to use the existing ExtensionApiTest and test
extension data?

On 2016/12/08 17:52:33, stevenjb wrote:
> I haven't done this in a while, but I think what you want
> is ExtensionBrowserTest.
> 
> 
> On Thu, Dec 8, 2016 at 10:37 AM, <mailto:jamescook@chromium.org> wrote:
> 
> > On 2016/12/08 17:22:34, stevenjb wrote:
> > > I think that would be better, thanks!
> > >
> > >
> > > On Thu, Dec 8, 2016 at 10:20 AM, <mailto:jamescook@chromium.org> wrote:
> > >
> > > > On 2016/12/08 17:07:11, stevenjb wrote:
> > > > > Hmm. I'm actually kind of not excited about this. Even though the
> > code is
> > > > > already testing the message center implementation (which it really
> > > > shouldn't),
> > > > > I'm not thrilled about adding more UI dependencies (and
> > specifically, ash
> > > > > dependencies to src/chrome code) to an 'apitest'.
> > > > >
> > > > > What is the long term plan for where ash + chrome integration tests
> > > > should
> > > > live?
> > > > >
> > > > > Regardless, this seems like it should be in a browser test for the
> > > > delegate,
> > > > not
> > > > > the apitest.
> > > >
> > > > I think ash/chrome integration tests will be browser tests in chrome.
> > > >
> > > > How about I make a new test for the delegate impl in c/b/ui/ash?
> > > >
> > > >
> > > > https://codereview.chromium.org/2558083003/
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> > Are you OK with the test of the delegate still deriving from
> > ExtensionApiTest
> > and using the same test extension? I need to load the test extension and
> > run it
> > to get it to register the SSID. There's probably a way to do this with an
> > ExtensionTest, but I don't know how off the top of my head and I would
> > have to
> > create a new test extension that duplicates some of the existing one.
> >
> >
> > https://codereview.chromium.org/2558083003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2016-12-08 17:59:18]

I think a duplicate test extension makes more sense here. At some point the
test extensions may diverge. The api test should just be ensuring that the
entire API itself is behaving properly, independent of the rest of the
system. In this case it's a very small API so the tests will probably look
about the same.


On Thu, Dec 8, 2016 at 10:56 AM, <jamescook@chromium.org> wrote:

> Sorry, ExtensionBrowserTest is what I meant. That might work, but I'll
> need a
> duplicate test extension that doesn't have all the extension API test
> stuff. Is
> that OK, or would you prefer me to use the existing ExtensionApiTest and
> test
> extension data?
>
> On 2016/12/08 17:52:33, stevenjb wrote:
> > I haven't done this in a while, but I think what you want
> > is ExtensionBrowserTest.
> >
> >
> > On Thu, Dec 8, 2016 at 10:37 AM, <mailto:jamescook@chromium.org> wrote:
> >
> > > On 2016/12/08 17:22:34, stevenjb wrote:
> > > > I think that would be better, thanks!
> > > >
> > > >
> > > > On Thu, Dec 8, 2016 at 10:20 AM, <mailto:jamescook@chromium.org>
> wrote:
> > > >
> > > > > On 2016/12/08 17:07:11, stevenjb wrote:
> > > > > > Hmm. I'm actually kind of not excited about this. Even though the
> > > code is
> > > > > > already testing the message center implementation (which it
> really
> > > > > shouldn't),
> > > > > > I'm not thrilled about adding more UI dependencies (and
> > > specifically, ash
> > > > > > dependencies to src/chrome code) to an 'apitest'.
> > > > > >
> > > > > > What is the long term plan for where ash + chrome integration
> tests
> > > > > should
> > > > > live?
> > > > > >
> > > > > > Regardless, this seems like it should be in a browser test for
> the
> > > > > delegate,
> > > > > not
> > > > > > the apitest.
> > > > >
> > > > > I think ash/chrome integration tests will be browser tests in
> chrome.
> > > > >
> > > > > How about I make a new test for the delegate impl in c/b/ui/ash?
> > > > >
> > > > >
> > > > > https://codereview.chromium.org/2558083003/
> > > > >
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google
> Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it,
> send an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > > Are you OK with the test of the delegate still deriving from
> > > ExtensionApiTest
> > > and using the same test extension? I need to load the test extension
> and
> > > run it
> > > to get it to register the SSID. There's probably a way to do this with
> an
> > > ExtensionTest, but I don't know how off the top of my head and I would
> > > have to
> > > create a new test extension that duplicates some of the existing one.
> > >
> > >
> > > https://codereview.chromium.org/2558083003/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
> https://codereview.chromium.org/2558083003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[James Cook, 2016-12-08 18:41:42]

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

[James Cook, 2016-12-08 18:42:00]

Steven, please take another look.

[commit-bot: I haz the power, 2016-12-08 18:42:34]

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

[stevenjb, 2016-12-08 18:45:23]

Ha, that wasn't so bad, good. Thanks for moving the test. Hopefully when I
re-factor the delegate I will fix the other api test to not rely on message
center (and add a similar integration tests).

LGTM

[James Cook, 2016-12-08 19:28:07]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2016-12-08 19:28:12]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-12-08 19:28: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-12-08 20:19:01]

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

[commit-bot: I haz the power, 2016-12-08 20:19:02]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[James Cook, 2016-12-08 20:23:44]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-12-08 20:24:36]

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-12-08 21:11:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481228624012890,
"parent_rev": "f58ce5682b244d7103c7e858051c3d58a2d8a336", "commit_rev":
"781f2fc1aee2e85de1531d2840aac9e45ceafde6"}

[commit-bot: I haz the power, 2016-12-08 21:11:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-08 21:14:45]

Description was changed from

==========
Add test for system tray network menu icon for extension-controlled networks

Eventually extension-controlled network information will come out of a mojo
service. This provides test coverage to make sure the icon shows up in the
system tray menu.

BUG=651157
TEST=chrome browser_tests
==========

to

==========
Add test for system tray network menu icon for extension-controlled networks

Eventually extension-controlled network information will come out of a mojo
service. This provides test coverage to make sure the icon shows up in the
system tray menu.

BUG=651157
TEST=chrome browser_tests

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

[commit-bot: I haz the power, 2016-12-08 21:14:46]

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

[yhirano, 2016-12-09 05:48:54]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2563943002/ by yhirano@chromium.org.

The reason for reverting is: NetworkingConfigDelegateChromeosTest.SystemTrayItem
is failing on Linux Chromium OS ASan LSan Tests: see
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=....