Issue 2694623016: chromeos: Makes AshTestBase/Helper target mash when appropriate

sky

Description was changed from ========== chromeos: Makes AshTestBase/Helper target mash when appropriate And gets a ...

3 years, 10 months ago (2017-02-17 18:22:01 UTC) #1

sky

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

3 years, 10 months ago (2017-02-17 18:22:10 UTC) #3

sky

reveman: exo jamescook: the rest https://codereview.chromium.org/2694623016/diff/1/ash/common/system/tray/tray_details_view_unittest.cc File ash/common/system/tray/tray_details_view_unittest.cc (right): https://codereview.chromium.org/2694623016/diff/1/ash/common/system/tray/tray_details_view_unittest.cc#newcode141 ash/common/system/tray/tray_details_view_unittest.cc:141: scoped_task_runner_ = This needs ...

3 years, 10 months ago (2017-02-17 18:22:15 UTC) #4

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-17 18:22:55 UTC) #5

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-17 20:09:34 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/324414)

3 years, 10 months ago (2017-02-17 20:09:36 UTC) #8

sky

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

3 years, 10 months ago (2017-02-17 23:07:19 UTC) #9

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-17 23:09:06 UTC) #10

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-18 01:13:10 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

3 years, 10 months ago (2017-02-18 01:13:11 UTC) #12

James Cook

I'm OOO next week -- apologies to you (and the follow-up reviewer) for not getting ...

3 years, 10 months ago (2017-02-18 02:23:21 UTC) #13

sky

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

3 years, 10 months ago (2017-02-18 17:58:10 UTC) #14

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-18 17:58:16 UTC) #15

sky

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

3 years, 10 months ago (2017-02-18 18:00:24 UTC) #16

sky

Description was changed from ========== chromeos: Makes AshTestBase/Helper target mash when appropriate And gets a ...

3 years, 10 months ago (2017-02-18 18:00:31 UTC) #17

sky

sky@chromium.org changed reviewers: + msw@chromium.org - jamescook@chromium.org

3 years, 10 months ago (2017-02-18 18:00:31 UTC) #18

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-18 18:00:36 UTC) #19

sky

Mike, James is out, could you take a look? https://codereview.chromium.org/2694623016/diff/20001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2694623016/diff/20001/ash/BUILD.gn#newcode1157 ash/BUILD.gn:1157: ...

3 years, 10 months ago (2017-02-18 18:00:57 UTC) #20

Mike, James is out, could you take a look?

https://codereview.chromium.org/2694623016/diff/20001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/2694623016/diff/20001/ash/BUILD.gn#newcode1157
ash/BUILD.gn:1157: source_set("common_unittests") {
On 2017/02/18 02:23:20, James Cook (OOO until Feb 27) wrote:
> Does this duplicate the list in ash/common/BUILD.gn? Can that file be deleted
> now?

It does indeed and includes one new file. The plan is to add more tests, but I
wanted to start with this first. ash/common/BUILD.gn has been nuked.

https://codereview.chromium.org/2694623016/diff/20001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2694623016/diff/20001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:127: session_state_delegate_ =
base::MakeUnique<SessionStateDelegateStub>();
On 2017/02/18 02:23:20, James Cook (OOO until Feb 27) wrote:
> Should this be session_state_delegate_for_testing_?

We were always creating SessionStateDelegateStub before. Maybe you're suggesting
I invert the meaning?

> It seems like the
> distinction is test vs. production (which uses Shell).
> 
> Also, it might be clearer if WmShellMus just took a unique_ptr<>
> session_state_delegate_for_test parameter.

I'm hoping this code is temporary, (the creation of the stub). You and Mike are
more familiar with this. I'm going to redirect this to Mike, who can suggest the
right approach.

https://codereview.chromium.org/2694623016/diff/20001/ash/mus/window_manager_...
File ash/mus/window_manager_unittest.cc (right):

https://codereview.chromium.org/2694623016/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_unittest.cc:88: ui::ContextFactory*
initial_context_factory_;
On 2017/02/18 02:23:20, James Cook (OOO until Feb 27) wrote:
> nit: = nullptr since constructor doesn't initialize

Done.

https://codereview.chromium.org/2694623016/diff/20001/ash/test/ash_test_helpe...
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2694623016/diff/20001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:75: DisplayInfo ParseDisplayBounds(const
std::string& spec) {
On 2017/02/18 02:23:21, James Cook (OOO until Feb 27) wrote:
> This seems like it should share code with
ManagedDisplayInfo::CreateFromSpec().
> Maybe use that for parsing and just copy out the fields you need?

Done.

https://codereview.chromium.org/2694623016/diff/20001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:182: blocking_pool_owner_ =
base::MakeUnique<base::SequencedWorkerPoolOwner>(
On 2017/02/18 02:23:21, James Cook (OOO until Feb 27) wrote:
> Could WindowManagerApplication handle the creation and ownership of the
blocking
> pool? It's not clear to me that it needs to be owned outside.

I think you may be right, but I would like to hold off on that for now. I did
remove the blocking_pool_owner_ from here those as AshTestEnvironment already
has it.

https://codereview.chromium.org/2694623016/diff/20001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:205: CreateRootWindowController("800x600", &next_x);
On 2017/02/18 02:23:21, James Cook (OOO until Feb 27) wrote:
> This code and functions below are very similar to the code in WmTestHelper. Is
> WmTestHelper going to go away? If not, is there any way to share?

Indeed. I would like to remove WmTestHelper and most likely AshTest.

https://codereview.chromium.org/2694623016/diff/20001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:218: }
On 2017/02/18 02:23:21, James Cook (OOO until Feb 27) wrote:
> This function is kind of long. Can you pull some of this stuff out into helper
> functions?

Done.

https://codereview.chromium.org/2694623016/diff/20001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:231: // the
On 2017/02/18 02:23:21, James Cook (OOO until Feb 27) wrote:
> nit: rewrap

Done.

https://codereview.chromium.org/2694623016/diff/20001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:348: return roots.size() < 2 ? display::Display()
On 2017/02/18 02:23:21, James Cook (OOO until Feb 27) wrote:
> Maybe it should be an error to try to get the secondary display if there is
only
> 1? I think that's what the old code did.

Done.

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-18 20:03:19 UTC) #21

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-18 20:03:20 UTC) #22

msw

Sorry, I have lots of qs... extra comments or CL desc might help. https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn File ...

3 years, 10 months ago (2017-02-19 21:49:20 UTC) #23

sky

https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn#newcode1103 ash/BUILD.gn:1103: "test/ash_test_suite.cc", On 2017/02/19 21:49:19, msw wrote: > Are these ...

3 years, 10 months ago (2017-02-21 17:06:51 UTC) #24

https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn#newcode1103
ash/BUILD.gn:1103: "test/ash_test_suite.cc",
On 2017/02/19 21:49:19, msw wrote:
> Are these (and the new //ui/gl:test_support dep) needed here? It seems like
they
> are already included in //ash/test:test_support_common, which should be pulled
> in transitively through existing deps: //ash/test:ash_with_aura_test_support
and
> //ash/test:test_support_with_content. Tangentially, it seems like these files
> aren't content-specific (the header includes ash_test_environment.h, not
> ash_test_environment_content.h, nothing in either that looks content related)

Mash has it's own TestSuite that is different than that of ash. Previously
ash_test_suite was in //ash/test:test_support_common. //ash:common_unittests
depends upon //ash/test:test_support_common. This means I can't keep
ash_test_suite in test_support_common and needed to move it to the test
executables that are using it (ash_content_unittests, ash_unittests and exo).

ash_test_suite depends upon //ui/gl:test_support, so it's needed as a dep here
(removing the dep results in gn --check errors).

https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn#newcode1176
ash/BUILD.gn:1176: "test/ash_test_environment_default.cc",
On 2017/02/19 21:49:19, msw wrote:
> Similarly, isn't this already included in test_support_without_content (a
direct
> dep here)?

Removed.

https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn#newcode1316
ash/BUILD.gn:1316: "test/ash_test_suite.cc",
On 2017/02/19 21:49:19, msw wrote:
> ditto question

Same answer as above.

https://codereview.chromium.org/2694623016/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/tray_details_view_unittest.cc (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view_unittest.cc:15: #include
"ash/common/wm_shell.h"
On 2017/02/19 21:49:20, msw wrote:
> nit: needed?

Done.

https://codereview.chromium.org/2694623016/diff/60001/ash/common/test/ash_tes...
File ash/common/test/ash_test.cc (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/common/test/ash_tes...
ash/common/test/ash_test.cc:108: if (WmShell::Get()->IsRunningInMash()) {
On 2017/02/19 21:49:20, msw wrote:
> Doesn't AshTestImplMus::SetSecondaryDisplayPlacement already do this?

Done.

https://codereview.chromium.org/2694623016/diff/60001/ash/display/window_tree...
File ash/display/window_tree_host_manager_unittest.cc (left):

https://codereview.chromium.org/2694623016/diff/60001/ash/display/window_tree...
ash/display/window_tree_host_manager_unittest.cc:188: display::Display
GetSecondaryDisplay() {
On 2017/02/19 21:49:20, msw wrote:
> Can you give some context for this? Do we need this on AshTestBase/Helper for
> parity with AshTest?

Exactly.

> (if so, why is it needed in this CL? afaict, nothing
> changes base classes here; is this just to prep for those changes?)

I added GetSecondaryDisplay() to AshTestBase, which manes this override is no
longer necessary.

> Does GetPrimaryDisplay also belong there? (It'd be nice to see more
consistency
> with those or just rely on display::Screen or similar)

Shell has a couple of functions for the primary window, but not the secondary.
It's common for tests to want the secondary, hence the convenience function.

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.h (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.h:45: bool create_session_state_delegate_stub);
On 2017/02/19 21:49:20, msw wrote:
> Can we just rely on the shell_delegate for this?
> ShellDelegateMus::CreateSessionStateDelegate already makes a stub.

Shell::Init() creates a SessionStateDelegate:

  session_state_delegate_.reset(
      wm_shell_->delegate()->CreateSessionStateDelegate());

In test code this ends up in TestShellDelegate::CreateSessionStateDelegate(),
which creates a TestSessionStateDelegate. Test code depends upon
TestSessionStateDelegate:

// static
TestSessionStateDelegate* AshTestHelper::GetTestSessionStateDelegate() {
  CHECK(WmShell::HasInstance());
  return static_cast<TestSessionStateDelegate*>(
      WmShell::Get()->GetSessionStateDelegate());
}

In other words the test code wants to use TestSessionStateDelegate, which I made
it get by having WmShellMus not create a SessionStateDelegate so that in:

SessionStateDelegate* WmShellMus::GetSessionStateDelegate() {
  return session_state_delegate_
             ? session_state_delegate_.get()
             : Shell::GetInstance()->session_state_delegate();
}

WmShellMus::session_state_delegate_ is null, and the code falls back to that of
Shell, which is TestSessionStateDelegate.

It seems the right fix is that both Shell and WmShellMus shouldn't create
SessionStateDelegate, only Shell. That's a bigger change though.

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/test/wm_test_he...
File ash/mus/test/wm_test_helper.cc (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:87: // General pattern is most places don't
uninitialize, so we have to explicitly
On 2017/02/19 21:49:20, msw wrote:
> Can you rephrase and clarify this comment? ie. The general pattern of what?
test
> fixtures? WmTestBase is the only user of this class, afaict, and its TearDown
> override resets the WmTestHelper instance, which already calls uninitialize,
so
> I'm a bit confused.

I tried improving the comment. Any better?

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/window_manager.h
File ash/mus/window_manager.h (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/window_manager....
ash/mus/window_manager.h:202: // See WmShellMus's constructor for details. Tests
may set to false.
On 2017/02/19 21:49:20, msw wrote:
> This is a bit confusing... tests may set this "create*for_test_" flag to
false,
> but otherwise it's true by default? I hope we can instead rely on using the
> correct shell delegate (directly above is a |shell_delegate_for_test_|
member),
> which does the right thing in CreateSessionStateDelegate.

Prior to my change the code always created a SessionStateDelegateStub. I needed
to turn that off in tests, hence naming it
create_session_state_delegate_stub_for_test_. I could invert the meaning, e.g.
dont_create_session_state_delegate_for_test_ if you prefer.

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/window_manager_...
File ash/mus/window_manager_unittest.cc (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/window_manager_...
ash/mus/window_manager_unittest.cc:73: // This test triggers a path that
clobbers the context_factory set on Env.
On 2017/02/19 21:49:20, msw wrote:
> Can you elaborate on the exact mechanism that clobbers the context factory?

Done.

> (Is
> it ash::Shell via AshTestHelper?) It's not obvious here. It would be far nicer
> if that mechanism were able to restore the original factory itself. (Could
> ash::Shell or AshTestHelper do that? call TerminateContextFactoryForTests() or
> similar?)

This code doesn't use any of the AshTest* classes, but it inherits the Env
created in MashTestSuite. TerminateContextFactoryForTests() wouldn't work well
in this case either as it doesn't reset env.

https://codereview.chromium.org/2694623016/diff/60001/ash/test/BUILD.gn
File ash/test/BUILD.gn (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/test/BUILD.gn#newco...
ash/test/BUILD.gn:144: "//ash/mus:lib",
On 2017/02/19 21:49:20, msw wrote:
> What needs this now?

As AshTestHelper now can target mus directly, it needs //ash/mus:lib.

https://codereview.chromium.org/2694623016/diff/60001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/test/ash_test_base....
ash/test/ash_test_base.cc:138: if (!WmShell::Get()->IsRunningInMash())
On 2017/02/19 21:49:20, msw wrote:
> nit: cite a bug? 637853?

Done.

https://codereview.chromium.org/2694623016/diff/60001/ash/test/ash_test_base....
ash/test/ash_test_base.cc:275: WmLookup::Get()
On 2017/02/19 21:49:20, msw wrote:
> optional nit: avoid WmLookup via WmShell::GetRootWindowForDisplayId?

Done.

https://codereview.chromium.org/2694623016/diff/60001/ash/test/ash_test_helpe...
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:59: // Assume if no env (which happens when
AshTestHelper is used outside of ash
On 2017/02/19 21:49:20, msw wrote:
> nit: "If there is no Env, which happens when ..., assume that ..."

Done.

https://codereview.chromium.org/2694623016/diff/60001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:144:
aura::test::EnvTestHelper().SetInputStateLookup(
On 2017/02/19 21:49:20, msw wrote:
> Is it okay that this might construct an Env instance here (in the
EnvTestHelper
> default ctor, by calling Env::GetInstance instead of
> Env::GetInstanceDontCreate)? I ask since your IsMash check specifically deals
> with the case where Env doesn't exist.

Env::GetInstance() DCHECKs that there is an instance. I had to add the
conditional in IsMash because I was calling IsMash from the constructor. I moved
that here, which means the conditional is no longer necessary.

https://codereview.chromium.org/2694623016/diff/60001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:154: // ScreenLayoutObserver is specific to cash.
On 2017/02/19 21:49:20, msw wrote:
> nit: cash -> classic Ash, ditto below

we're going to start using cash in file names soon, but I can hold off for now.
Done.

https://codereview.chromium.org/2694623016/diff/60001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:274: void AshTestHelper::CreateMashWindowManager() {
On 2017/02/19 21:49:20, msw wrote:
> nit: CHECK(IsMash()); at top

Done.

https://codereview.chromium.org/2694623016/diff/60001/ash/test/ash_test_helpe...
ash/test/ash_test_helper.cc:300: void AshTestHelper::CreateShell() {
On 2017/02/19 21:49:20, msw wrote:
> nit: CHECK(!IsMash()); at top

Done.

https://codereview.chromium.org/2694623016/diff/60001/components/exo/BUILD.gn
File components/exo/BUILD.gn (right):

https://codereview.chromium.org/2694623016/diff/60001/components/exo/BUILD.gn...
components/exo/BUILD.gn:158: "../../ash/test/ash_test_suite.cc",
On 2017/02/19 21:49:20, msw wrote:
> ditto q

See earlier one.

sky

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

3 years, 10 months ago (2017-02-21 17:06:52 UTC) #25

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-21 17:07:31 UTC) #26

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-21 17:10:05 UTC) #27

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/157130) ios-simulator on ...

3 years, 10 months ago (2017-02-21 17:10:06 UTC) #28

sky

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

3 years, 10 months ago (2017-02-21 17:45:47 UTC) #29

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-21 17:46:27 UTC) #30

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-21 21:06:44 UTC) #31

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

3 years, 10 months ago (2017-02-21 21:06:45 UTC) #32

sky

sky@chromium.org changed reviewers: + derat@chromium.org - msw@chromium.org

3 years, 10 months ago (2017-02-21 23:01:20 UTC) #33

sky

As Mike is now out for a couple of days I'm passing to Dan. Dan, ...

3 years, 10 months ago (2017-02-21 23:01:53 UTC) #34

msw

lgtm with a nit and minor qs https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn#newcode1103 ash/BUILD.gn:1103: "test/ash_test_suite.cc", On ...

3 years, 10 months ago (2017-02-22 06:25:48 UTC) #36

lgtm with a nit and minor qs

https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn#newcode1103
ash/BUILD.gn:1103: "test/ash_test_suite.cc",
On 2017/02/21 17:06:50, sky wrote:
> On 2017/02/19 21:49:19, msw wrote:
> > Are these (and the new //ui/gl:test_support dep) needed here? It seems like
> they
> > are already included in //ash/test:test_support_common, which should be
pulled
> > in transitively through existing deps: //ash/test:ash_with_aura_test_support
> and
> > //ash/test:test_support_with_content. Tangentially, it seems like these
files
> > aren't content-specific (the header includes ash_test_environment.h, not
> > ash_test_environment_content.h, nothing in either that looks content
related)
> 
> Mash has it's own TestSuite that is different than that of ash. Previously
> ash_test_suite was in //ash/test:test_support_common. //ash:common_unittests
> depends upon //ash/test:test_support_common. This means I can't keep
> ash_test_suite in test_support_common and needed to move it to the test
> executables that are using it (ash_content_unittests, ash_unittests and exo).
> 
> ash_test_suite depends upon //ui/gl:test_support, so it's needed as a dep here
> (removing the dep results in gn --check errors).

Hmm, couldn't two separate TestSuite subclasses be included in a target (via
deps and sources), and then each test target could explicitly instantiate and
use the one it needs? I guess I don't understand why we need to move
ash_test_suite out of //ash/test:test_support_common, unless you're trying to
avoid including it in targets where it's simply not used.

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.h (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.h:45: bool create_session_state_delegate_stub);
On 2017/02/21 17:06:50, sky wrote:
> On 2017/02/19 21:49:20, msw wrote:
> > Can we just rely on the shell_delegate for this?
> > ShellDelegateMus::CreateSessionStateDelegate already makes a stub.
> 
> Shell::Init() creates a SessionStateDelegate:
> 
>   session_state_delegate_.reset(
>       wm_shell_->delegate()->CreateSessionStateDelegate());
> 
> In test code this ends up in TestShellDelegate::CreateSessionStateDelegate(),
> which creates a TestSessionStateDelegate. Test code depends upon
> TestSessionStateDelegate:
> 
> // static
> TestSessionStateDelegate* AshTestHelper::GetTestSessionStateDelegate() {
>   CHECK(WmShell::HasInstance());
>   return static_cast<TestSessionStateDelegate*>(
>       WmShell::Get()->GetSessionStateDelegate());
> }
> 
> In other words the test code wants to use TestSessionStateDelegate, which I
made
> it get by having WmShellMus not create a SessionStateDelegate so that in:
> 
> SessionStateDelegate* WmShellMus::GetSessionStateDelegate() {
>   return session_state_delegate_
>              ? session_state_delegate_.get()
>              : Shell::GetInstance()->session_state_delegate();
> }
> 
> WmShellMus::session_state_delegate_ is null, and the code falls back to that
of
> Shell, which is TestSessionStateDelegate.
> 
> It seems the right fix is that both Shell and WmShellMus shouldn't create
> SessionStateDelegate, only Shell. That's a bigger change though.

Got it, it'd be nice to summarize that in a bug or comment. It's slightly
confusing without that explainer.

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/test/wm_test_he...
File ash/mus/test/wm_test_helper.cc (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/test/wm_test_he...
ash/mus/test/wm_test_helper.cc:87: // General pattern is most places don't
uninitialize, so we have to explicitly
On 2017/02/21 17:06:50, sky wrote:
> On 2017/02/19 21:49:20, msw wrote:
> > Can you rephrase and clarify this comment? ie. The general pattern of what?
> test
> > fixtures? WmTestBase is the only user of this class, afaict, and its
TearDown
> > override resets the WmTestHelper instance, which already calls uninitialize,
> so
> > I'm a bit confused.
> 
> I tried improving the comment. Any better?

Yes, that makes more sense now.

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/window_manager.h
File ash/mus/window_manager.h (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/mus/window_manager....
ash/mus/window_manager.h:202: // See WmShellMus's constructor for details. Tests
may set to false.
On 2017/02/21 17:06:50, sky wrote:
> On 2017/02/19 21:49:20, msw wrote:
> > This is a bit confusing... tests may set this "create*for_test_" flag to
> false,
> > but otherwise it's true by default? I hope we can instead rely on using the
> > correct shell delegate (directly above is a |shell_delegate_for_test_|
> member),
> > which does the right thing in CreateSessionStateDelegate.
> 
> Prior to my change the code always created a SessionStateDelegateStub. I
needed
> to turn that off in tests, hence naming it
> create_session_state_delegate_stub_for_test_. I could invert the meaning, e.g.
> dont_create_session_state_delegate_for_test_ if you prefer.

No, it's fine as-is... hopefully the stub will go away soon.

https://codereview.chromium.org/2694623016/diff/100001/ash/mus/window_manager...
File ash/mus/window_manager_unittest.cc (right):

https://codereview.chromium.org/2694623016/diff/100001/ash/mus/window_manager...
ash/mus/window_manager_unittest.cc:74: // aura::WindowTreeClient resetting the
context_factory on Env. As all
q: Should WindowTreeClient's dtor reset the Env instance?

https://codereview.chromium.org/2694623016/diff/100001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/2694623016/diff/100001/ash/test/ash_test_base...
ash/test/ash_test_base.cc:13: #include "ash/common/wm_lookup.h"
nit: no longer needed

sky

https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn#newcode1103 ash/BUILD.gn:1103: "test/ash_test_suite.cc", On 2017/02/22 06:25:48, msw (vacation feb 21 maybe ...

3 years, 10 months ago (2017-02-22 16:32:05 UTC) #37

https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn
File ash/BUILD.gn (right):

https://codereview.chromium.org/2694623016/diff/60001/ash/BUILD.gn#newcode1103
ash/BUILD.gn:1103: "test/ash_test_suite.cc",
On 2017/02/22 06:25:48, msw (vacation feb 21 maybe 22) wrote:
> On 2017/02/21 17:06:50, sky wrote:
> > On 2017/02/19 21:49:19, msw wrote:
> > > Are these (and the new //ui/gl:test_support dep) needed here? It seems
like
> > they
> > > are already included in //ash/test:test_support_common, which should be
> pulled
> > > in transitively through existing deps:
//ash/test:ash_with_aura_test_support
> > and
> > > //ash/test:test_support_with_content. Tangentially, it seems like these
> files
> > > aren't content-specific (the header includes ash_test_environment.h, not
> > > ash_test_environment_content.h, nothing in either that looks content
> related)
> > 
> > Mash has it's own TestSuite that is different than that of ash. Previously
> > ash_test_suite was in //ash/test:test_support_common. //ash:common_unittests
> > depends upon //ash/test:test_support_common. This means I can't keep
> > ash_test_suite in test_support_common and needed to move it to the test
> > executables that are using it (ash_content_unittests, ash_unittests and
exo).
> > 
> > ash_test_suite depends upon //ui/gl:test_support, so it's needed as a dep
here
> > (removing the dep results in gn --check errors).
> 
> Hmm, couldn't two separate TestSuite subclasses be included in a target (via
> deps and sources), and then each test target could explicitly instantiate and
> use the one it needs? I guess I don't understand why we need to move
> ash_test_suite out of //ash/test:test_support_common, unless you're trying to
> avoid including it in targets where it's simply not used.

I think you're suggesting a separate target for the test suites? ash_test_suite
includes some files in ash/test. This means I would need to restructure
test_support_common, which didn't seem worth it.

https://codereview.chromium.org/2694623016/diff/100001/ash/mus/window_manager...
File ash/mus/window_manager_unittest.cc (right):

https://codereview.chromium.org/2694623016/diff/100001/ash/mus/window_manager...
ash/mus/window_manager_unittest.cc:74: // aura::WindowTreeClient resetting the
context_factory on Env. As all
On 2017/02/22 06:25:48, msw (vacation feb 21 maybe 22) wrote:
> q: Should WindowTreeClient's dtor reset the Env instance?

If you mean reset the context factor, that sounds like a good idea. I'll do it
separately though.

https://codereview.chromium.org/2694623016/diff/100001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/2694623016/diff/100001/ash/test/ash_test_base...
ash/test/ash_test_base.cc:13: #include "ash/common/wm_lookup.h"
On 2017/02/22 06:25:48, msw (vacation feb 21 maybe 22) wrote:
> nit: no longer needed

Done.

sky

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, msw@chromium.org Link to ...

3 years, 10 months ago (2017-02-22 16:32:22 UTC) #39

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2694623016/120001

3 years, 10 months ago (2017-02-22 16:32:41 UTC) #40

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487781141612660, "parent_rev": "e96528b1b2250ef51580a706dd7950293944cfd3", "commit_rev": "d06586de254e7ecfd8d3620a45253faaf520334e"}

3 years, 10 months ago (2017-02-22 17:46:42 UTC) #43

commit-bot: I haz the power

Description was changed from ========== chromeos: Makes AshTestBase/Helper target mash when appropriate And gets a ...

3 years, 10 months ago (2017-02-22 17:47:14 UTC) #44

commit-bot: I haz the power

Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d06586de254e7ecfd8d3620a45253faaf520334e

3 years, 10 months ago (2017-02-22 17:47:16 UTC) #45

sky

3 years, 10 months ago (2017-02-22 17:53:50 UTC) #46

Message was sent while issue was closed.

https://codereview.chromium.org/2694623016/diff/100001/ash/mus/window_manager...
File ash/mus/window_manager_unittest.cc (right):

https://codereview.chromium.org/2694623016/diff/100001/ash/mus/window_manager...
ash/mus/window_manager_unittest.cc:74: // aura::WindowTreeClient resetting the
context_factory on Env. As all
On 2017/02/22 16:32:05, sky wrote:
> On 2017/02/22 06:25:48, msw (vacation feb 21 maybe 22) wrote:
> > q: Should WindowTreeClient's dtor reset the Env instance?
> 
> If you mean reset the context factor, that sounds like a good idea. I'll do it
> separately though.

Patch for reset context factory in ~WindowTreeClient is here:
https://codereview.chromium.org/2706403004/ .

Issue 2694623016: chromeos: Makes AshTestBase/Helper target mash when appropriate (Closed)

Description

Patch Set 1 #

Patch Set 2 : cleanup #

Patch Set 3 : feedback #

Patch Set 4 : comment #

Patch Set 5 : feedback #

Patch Set 6 : format #

Patch Set 7 : feedback #

Messages