New issue
Advanced search Search tips

Issue 654495 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Closed: Feb 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Convert System tray unittests to use AshTest

Project Member Reported by sky@chromium.org, Oct 10 2016

Issue description

They are currently using AshTestBase. Once converted they should move to ash/common/BUILD.gn.

 
Cc: -jamescook@chromium.org
Owner: jamescook@chromium.org
Status: Assigned (was: Untriaged)
Summary: Convert System tray unittests to use AshTest (was: Convert System unittests to use AshTest)
I'll start looking at these.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec

commit 9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec
Author: jamescook <jamescook@chromium.org>
Date: Wed Oct 12 19:28:50 2016

mash: Port TrayUpdateTest to AshTest

* Add AshTest::GetPrimarySystemTray().
* Set a TestSystemTrayDelegate in mash unittests because this test relies on
there not being an update available at test start, but SystemTrayDelegateMus
is based on DefaultSystemTrayDelegate, which simulates an update being ready.

BUG= 654495 
TEST=ash_unittests, mash_unittests

Review-Url: https://codereview.chromium.org/2408273002
Cr-Commit-Position: refs/heads/master@{#424822}

[modify] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/BUILD.gn
[modify] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/common/BUILD.gn
[modify] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/common/system/update/tray_update_unittest.cc
[modify] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/common/test/BUILD.gn
[modify] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/common/test/ash_test.cc
[modify] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/common/test/ash_test.h
[add] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/common/test/wm_shell_test_api.cc
[add] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/common/test/wm_shell_test_api.h
[modify] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/common/wm_shell.cc
[modify] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/common/wm_shell.h
[modify] https://crrev.com/9d7d8eafd909e356d87b94e3fd2fc7e3d7d78aec/ash/mus/test/wm_test_helper.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/662f55a483bd54e2566782d9b064d70cd7eb210a

commit 662f55a483bd54e2566782d9b064d70cd7eb210a
Author: jamescook <jamescook@chromium.org>
Date: Thu Oct 13 04:30:30 2016

mash: Convert TimeViewTest to AshTest

It only has ash/common dependencies.

BUG= 654495 
TEST=ash_unittests, mash_unittests

Review-Url: https://codereview.chromium.org/2411923002
Cr-Commit-Position: refs/heads/master@{#424957}

[modify] https://crrev.com/662f55a483bd54e2566782d9b064d70cd7eb210a/ash/BUILD.gn
[modify] https://crrev.com/662f55a483bd54e2566782d9b064d70cd7eb210a/ash/common/BUILD.gn
[modify] https://crrev.com/662f55a483bd54e2566782d9b064d70cd7eb210a/ash/common/system/date/date_view_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0ded2732e3832c3b1b060098330cf14096c4342

commit d0ded2732e3832c3b1b060098330cf14096c4342
Author: jamescook <jamescook@chromium.org>
Date: Fri Oct 14 17:46:30 2016

mustash: Convert TraySupervisedUserTest to AshTest

It only uses common types.

* Add AshTest::GetSystemTrayDelegate, which is used in about 10 places in
other tests.
* Reset the initial login status directly in this test. This is the only
call site for SetInitialLoginStatus(). Continuing to do it in AshTestHelper
would mean I would have to duplicate that call in WmTestHelper, which seems
worse than what I did here.

BUG= 654495 
TEST=ash_unittests, mash_unittests

Review-Url: https://codereview.chromium.org/2420783002
Cr-Commit-Position: refs/heads/master@{#425382}

[modify] https://crrev.com/d0ded2732e3832c3b1b060098330cf14096c4342/ash/BUILD.gn
[modify] https://crrev.com/d0ded2732e3832c3b1b060098330cf14096c4342/ash/common/BUILD.gn
[modify] https://crrev.com/d0ded2732e3832c3b1b060098330cf14096c4342/ash/common/system/chromeos/supervised/tray_supervised_user_unittest.cc
[modify] https://crrev.com/d0ded2732e3832c3b1b060098330cf14096c4342/ash/common/test/ash_test.cc
[modify] https://crrev.com/d0ded2732e3832c3b1b060098330cf14096c4342/ash/common/test/ash_test.h
[modify] https://crrev.com/d0ded2732e3832c3b1b060098330cf14096c4342/ash/common/test/test_system_tray_delegate.cc
[modify] https://crrev.com/d0ded2732e3832c3b1b060098330cf14096c4342/ash/common/test/test_system_tray_delegate.h
[modify] https://crrev.com/d0ded2732e3832c3b1b060098330cf14096c4342/ash/test/ash_test_helper.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/88e3718e8d9b4a20b112ec5258739619d085f6f8

commit 88e3718e8d9b4a20b112ec5258739619d085f6f8
Author: jamescook <jamescook@chromium.org>
Date: Fri Oct 14 19:05:19 2016

mutash: Convert ScreenShareTest/ScreenCaptureTest to AshTest

The conversion is mechanical, but exposed the fact that we initialize
chromeos::NetworkHandler in mash_unittests but do not in ash_unittests.
This exposed a crash in the TrayVpn code. Since initializing NetworkHandler
more closely matches how ash runs in production, I chose to skip showing the
VPN tray item if no VPN delegate is available.

BUG= 654495 
TEST=ash_unittests, mash_unittests

Review-Url: https://codereview.chromium.org/2422453002
Cr-Commit-Position: refs/heads/master@{#425412}

[modify] https://crrev.com/88e3718e8d9b4a20b112ec5258739619d085f6f8/ash/BUILD.gn
[modify] https://crrev.com/88e3718e8d9b4a20b112ec5258739619d085f6f8/ash/common/BUILD.gn
[modify] https://crrev.com/88e3718e8d9b4a20b112ec5258739619d085f6f8/ash/common/system/chromeos/network/tray_vpn.cc
[modify] https://crrev.com/88e3718e8d9b4a20b112ec5258739619d085f6f8/ash/common/system/chromeos/screen_security/screen_tray_item_unittest.cc

Comment 6 by sky@chromium.org, Feb 23 2017

We can now use AshTestBase for mash, so no point in doing this. James, can this be moved to fixed now? 
Status: Fixed (was: Assigned)

Comment 8 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 9 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 11 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)

Sign in to add a comment