Parameterize Ash unittests to pass with material design enabled

Parameterize Ash unittests to pass with material design enabled

Parameterize test fixtures in ash_unittests
to run with the three modes used in Ash
material design (non-material, material stable,
and material experimental); see the runtime flag
--ash-md.

Also introduce the AshMdTestBase class to aid
in modifying test expectations across the
parameterized test runs.

BUG=617296
TEST=ash_unittests

Committed: https://crrev.com/0b7905b2457ab5222a853823b3ba817d4397f7a5
Cr-Commit-Position: refs/heads/master@{#401420}

[tdanderson, 2016-06-16 00:46:45]

Description was changed from

==========
[WIP] Parameterize Ash unittests to pass with material design enabled

WIP, not for review.

BUG=617296
TEST=ash_unittests
==========

to

==========
[WIP] Parameterize Ash unittests to pass with material design enabled

WIP, not for formal review.

BUG=617296
TEST=ash_unittests
==========

[tdanderson, 2016-06-16 00:50:03]

tdanderson@chromium.org changed reviewers:
+ jamescook@chromium.org, varkha@chromium.org

[tdanderson, 2016-06-16 00:50:04]

Valery and James, can I please get your initial feedback on this CL? This does
not yet address all of the tests which fail with the --ash-md flag enabled
(there are ~10 more test fixtures which I still need to change) but what you see
in Patch Set 2 is a representative sample of what the rest of the changes will
be like.

[varkha, 2016-06-16 02:55:05]

https://codereview.chromium.org/2067223003/diff/20001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/2067223003/diff/20001/ash/display/display_man...
ash/display/display_manager_unittest.cc:60: public
testing::WithParamInterface<ash::MaterialDesignController::Mode> {
Here and in other fixtures that inherit from testing::WithParamInterface<Mode>:
Does it make sense to derive from AshMDTestBase and inherit the
material_design_state_ and offset?

[James Cook, 2016-06-16 16:08:56]

Overall approach seems fine.

https://codereview.chromium.org/2067223003/diff/20001/ash/dip_unittest.cc
File ash/dip_unittest.cc (right):

https://codereview.chromium.org/2067223003/diff/20001/ash/dip_unittest.cc#new...
ash/dip_unittest.cc:32: typedef ash::test::AshMDTestBase DIPTest;
nit: using DIPTest =

https://codereview.chromium.org/2067223003/diff/20001/ash/dip_unittest.cc#new...
ash/dip_unittest.cc:34: // Note: First argument is optional and intentionally
left blank.
Why leave this out? Most other parameterized tests use it.

https://codereview.chromium.org/2067223003/diff/20001/ash/dip_unittest.cc#new...
ash/dip_unittest.cc:39:
testing::Values(ash::MaterialDesignController::NON_MATERIAL,
nit: no ash::

https://codereview.chromium.org/2067223003/diff/20001/ash/dip_unittest.cc#new...
ash/dip_unittest.cc:45: #if defined(OS_CHROMEOS)
Do you have to rework the #ifdefs this way? (Because of the parameterization?) 
If not, it might be useful to keep the tests compiling on Windows, even if they
are disabled.

https://codereview.chromium.org/2067223003/diff/20001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/2067223003/diff/20001/ash/display/display_man...
ash/display/display_manager_unittest.cc:60: public
testing::WithParamInterface<ash::MaterialDesignController::Mode> {
On 2016/06/16 02:55:05, varkha wrote:
> Here and in other fixtures that inherit from
testing::WithParamInterface<Mode>:
> Does it make sense to derive from AshMDTestBase and inherit the
> material_design_state_ and offset?

+1

https://codereview.chromium.org/2067223003/diff/20001/ash/test/ash_md_test_ba...
File ash/test/ash_md_test_base.cc (right):

https://codereview.chromium.org/2067223003/diff/20001/ash/test/ash_md_test_ba...
ash/test/ash_md_test_base.cc:11: : md_maximized_window_height_offset_(0) {
nit: init in header

https://codereview.chromium.org/2067223003/diff/20001/ash/test/ash_md_test_ba...
ash/test/ash_md_test_base.cc:22: ash::MaterialDesignController::IsMaterial() ?
-1 : 0;
optional: maybe compute this based on the constants themselves? or at least
reference them in a comment?

https://codereview.chromium.org/2067223003/diff/20001/ash/test/ash_md_test_ba...
File ash/test/ash_md_test_base.h (right):

https://codereview.chromium.org/2067223003/diff/20001/ash/test/ash_md_test_ba...
ash/test/ash_md_test_base.h:16: : public ash::test::AshTestBase,
nit: no ash::test::

[tdanderson, 2016-06-21 19:52:21]

Hi James, can you please take another look? In particular please see my
comment/question in window_state_unittest.cc.

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest.cc
File ash/dip_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest...
ash/dip_unittest.cc:32: typedef ash::test::AshMDTestBase DIPTest;
On 2016/06/16 16:08:56, James Cook wrote:
> nit: using DIPTest =

Done.

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest...
ash/dip_unittest.cc:34: // Note: First argument is optional and intentionally
left blank.
On 2016/06/16 16:08:56, James Cook wrote:
> Why leave this out? Most other parameterized tests use it.

It seems this first argument is only useful to disambiguate between runs which
have two or more parameters (i.e., if INSTANTIATE_TEST_CASE_P is used two or
more times for the same test fixture). Please correct me if I am mistaken on
this.

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest...
ash/dip_unittest.cc:39:
testing::Values(ash::MaterialDesignController::NON_MATERIAL,
On 2016/06/16 16:08:56, James Cook wrote:
> nit: no ash::

Done.

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest...
ash/dip_unittest.cc:45: #if defined(OS_CHROMEOS)
On 2016/06/16 16:08:56, James Cook wrote:
> Do you have to rework the #ifdefs this way? (Because of the parameterization?)

> If not, it might be useful to keep the tests compiling on Windows, even if
they
> are disabled.

To the best of my understanding this is required in order to get around the
parameterization. Though if I am missing something and you know of another way,
please let me know. (I tried WRAPPED_INSTANTIATE_TEST_CASE_P but without any
luck.)

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/display/disp...
File ash/display/display_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/display/disp...
ash/display/display_manager_unittest.cc:60: public
testing::WithParamInterface<ash::MaterialDesignController::Mode> {
On 2016/06/16 16:08:56, James Cook wrote:
> On 2016/06/16 02:55:05, varkha wrote:
> > Here and in other fixtures that inherit from
> testing::WithParamInterface<Mode>:
> > Does it make sense to derive from AshMDTestBase and inherit the
> > material_design_state_ and offset?
> 
> +1

Done.

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/test/ash_md_...
File ash/test/ash_md_test_base.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/test/ash_md_...
ash/test/ash_md_test_base.cc:11: : md_maximized_window_height_offset_(0) {
On 2016/06/16 16:08:56, James Cook wrote:
> nit: init in header

Can you clarify what you mean here? Inlining a ctor looks to be forbidden by our
style guide, and assigning a value of 0 on line 34 seems a bit strange to do.

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/test/ash_md_...
ash/test/ash_md_test_base.cc:22: ash::MaterialDesignController::IsMaterial() ?
-1 : 0;
On 2016/06/16 16:08:56, James Cook wrote:
> optional: maybe compute this based on the constants themselves? or at least
> reference them in a comment?

This is a great idea, done.

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/test/ash_md_...
File ash/test/ash_md_test_base.h (right):

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/test/ash_md_...
ash/test/ash_md_test_base.h:16: : public ash::test::AshTestBase,
On 2016/06/16 16:08:56, James Cook wrote:
> nit: no ash::test::

Done.

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/shelf/shelf_...
File ash/shelf/shelf_widget_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/shelf/shelf_...
ash/shelf/shelf_widget_unittest.cc:206: TEST_P(ShelfWidgetTest,
ShelfEdgeOverlappingWindowHitTestMouse) {
ditto to the comments I made in WindowStateTest.RestoredWindowBoundsShrink.

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
File ash/wm/window_state_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
ash/wm/window_state_unittest.cc:331: TEST_P(WindowStateTest,
RestoredWindowBoundsShrink) {
This test fails on CrOS with GetParam() == 2 (MATERIAL_EXPERIMENTAL) since the
|work_area| bounds on line 335 are incorrectly being reported as 0,0 800x553
(should be the same as the maximized window bounds, which are 0,0 800x552).

Assuming this was due to stale display info in between test runs, in patch set 2
I added UpdateDisplay("0+0-600x900") to the top of this test. This causes the
test to pass on CrOS, but it fails on the Windows bots (again for the same
reason that the work area is incorrectly reported as 0,0 800x553).

Do you have any insight into how best to proceed with this? Looking at
DisplayManagerTestApi::UpdateDisplay(), I see that there is some code commented
out for OS_WIN marked with a TODO(msw). Unless you have any ideas offhand, I
plan on pinging msw@ to ask about that code.

[James Cook, 2016-06-21 22:17:54]

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest.cc
File ash/dip_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest...
ash/dip_unittest.cc:34: // Note: First argument is optional and intentionally
left blank.
On 2016/06/21 19:52:20, tdanderson wrote:
> On 2016/06/16 16:08:56, James Cook wrote:
> > Why leave this out? Most other parameterized tests use it.
> 
> It seems this first argument is only useful to disambiguate between runs which
> have two or more parameters (i.e., if INSTANTIATE_TEST_CASE_P is used two or
> more times for the same test fixture). Please correct me if I am mistaken on
> this.

OK, I'm fine with leaving it blank. It's just awkward that you need a 2-line
comment each time explaining why it's blank. You could also either add a junk
prefix ("MD") or an inline comment /* intentionally blank prefix */

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest...
ash/dip_unittest.cc:45: #if defined(OS_CHROMEOS)
On 2016/06/21 19:52:20, tdanderson wrote:
> On 2016/06/16 16:08:56, James Cook wrote:
> > Do you have to rework the #ifdefs this way? (Because of the
parameterization?)
> 
> > If not, it might be useful to keep the tests compiling on Windows, even if
> they
> > are disabled.
> 
> To the best of my understanding this is required in order to get around the
> parameterization. Though if I am missing something and you know of another
way,
> please let me know. (I tried WRAPPED_INSTANTIATE_TEST_CASE_P but without any
> luck.)

Huh, it's weird that WRAPPED_INSTANTIATE_TEST_CASE_P doesn't work. That seems
like exactly what you need:
https://cs-staging.chromium.org/chromium/src/net/cert/cert_verify_proc_unitte...

I wonder if it's because of the typedef / using.

Maybe give it one more try, and if it still doesn't work, just #ifdef it all out
like you're doing now.

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/test/ash_md_...
File ash/test/ash_md_test_base.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/test/ash_md_...
ash/test/ash_md_test_base.cc:11: : md_maximized_window_height_offset_(0) {
On 2016/06/21 19:52:21, tdanderson wrote:
> On 2016/06/16 16:08:56, James Cook wrote:
> > nit: init in header
> 
> Can you clarify what you mean here? Inlining a ctor looks to be forbidden by
our
> style guide, and assigning a value of 0 on line 34 seems a bit strange to do.

I mean "int md_maximized_window_height_offset_ = 0;" in the header. Given that
there is no complex initialization going on here, initializing in the header
puts the default value closer to the declaration and makes it easier to see that
all POD members are initialized.

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/dip_unittest.cc
File ash/dip_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/dip_unittest...
ash/dip_unittest.cc:37: ,
see previous comment

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/test/ash_md_...
File ash/test/ash_md_test_base.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/test/ash_md_...
ash/test/ash_md_test_base.cc:23: md_maximized_window_height_offset_ =
default_shelf_size - md_state_shelf_size;
Nice.

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
File ash/wm/window_state_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
ash/wm/window_state_unittest.cc:331: TEST_P(WindowStateTest,
RestoredWindowBoundsShrink) {
On 2016/06/21 19:52:21, tdanderson wrote:
> This test fails on CrOS with GetParam() == 2 (MATERIAL_EXPERIMENTAL) since the
> |work_area| bounds on line 335 are incorrectly being reported as 0,0 800x553
> (should be the same as the maximized window bounds, which are 0,0 800x552).
> 
> Assuming this was due to stale display info in between test runs, in patch set
2
> I added UpdateDisplay("0+0-600x900") to the top of this test. This causes the
> test to pass on CrOS, but it fails on the Windows bots (again for the same
> reason that the work area is incorrectly reported as 0,0 800x553).
> 
> Do you have any insight into how best to proceed with this? Looking at
> DisplayManagerTestApi::UpdateDisplay(), I see that there is some code
commented
> out for OS_WIN marked with a TODO(msw). Unless you have any ideas offhand, I
> plan on pinging msw@ to ask about that code.

Hmm. I'm not sure what you mean by "stale display info". Each individual test
should construct a new AshTestBase and AshTestHelper. It's conceivable that
there are static/global variables that are set that persist between tests.

It seems kinda weird that if you make the screen 900 pixels tall via 0+0-600x900
that windows sees a work area ~550 pixels tall.

Does this repro for you locally on cros?  If not, oshima knows how to exactly
repro the runtime environment from the bots on a local machine. I approximate it
with xvfb-run -s "-screen 0 1280x800x24" out/Default/ash_unittests

I would ping msw too.

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
ash/wm/window_state_unittest.cc:377: // Exiting fullscreen will update the
maximized widnow to the work area.
nit: while you're here, widnow -> window

[varkha, 2016-06-22 16:40:18]

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
File ash/wm/window_state_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
ash/wm/window_state_unittest.cc:331: TEST_P(WindowStateTest,
RestoredWindowBoundsShrink) {
On 2016/06/21 22:17:54, James Cook wrote:
> On 2016/06/21 19:52:21, tdanderson wrote:
> > This test fails on CrOS with GetParam() == 2 (MATERIAL_EXPERIMENTAL) since
the
> > |work_area| bounds on line 335 are incorrectly being reported as 0,0 800x553
> > (should be the same as the maximized window bounds, which are 0,0 800x552).
> > 
> > Assuming this was due to stale display info in between test runs, in patch
set
> 2
> > I added UpdateDisplay("0+0-600x900") to the top of this test. This causes
the
> > test to pass on CrOS, but it fails on the Windows bots (again for the same
> > reason that the work area is incorrectly reported as 0,0 800x553).
> > 
> > Do you have any insight into how best to proceed with this? Looking at
> > DisplayManagerTestApi::UpdateDisplay(), I see that there is some code
> commented
> > out for OS_WIN marked with a TODO(msw). Unless you have any ideas offhand, I
> > plan on pinging msw@ to ask about that code.
> 
> Hmm. I'm not sure what you mean by "stale display info". Each individual test
> should construct a new AshTestBase and AshTestHelper. It's conceivable that
> there are static/global variables that are set that persist between tests.
> 
> It seems kinda weird that if you make the screen 900 pixels tall via
0+0-600x900
> that windows sees a work area ~550 pixels tall.
> 
> Does this repro for you locally on cros?  If not, oshima knows how to exactly
> repro the runtime environment from the bots on a local machine. I approximate
it
> with xvfb-run -s "-screen 0 1280x800x24" out/Default/ash_unittests
> 
> I would ping msw too.

Maybe you need AshTestBase::SupportsHostWindowResize() which returns |false| on
Windows since you call UpdateDisplay in the test.

[tdanderson, 2016-06-22 16:51:56]

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest.cc
File ash/dip_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest...
ash/dip_unittest.cc:34: // Note: First argument is optional and intentionally
left blank.
On 2016/06/21 22:17:54, James Cook wrote:
> On 2016/06/21 19:52:20, tdanderson wrote:
> > On 2016/06/16 16:08:56, James Cook wrote:
> > > Why leave this out? Most other parameterized tests use it.
> > 
> > It seems this first argument is only useful to disambiguate between runs
which
> > have two or more parameters (i.e., if INSTANTIATE_TEST_CASE_P is used two or
> > more times for the same test fixture). Please correct me if I am mistaken on
> > this.
> 
> OK, I'm fine with leaving it blank. It's just awkward that you need a 2-line
> comment each time explaining why it's blank. You could also either add a junk
> prefix ("MD") or an inline comment /* intentionally blank prefix */

Changed everywhere to be consistent with what you LGTM'ed in
https://codereview.chromium.org/2063633002/

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/dip_unittest...
ash/dip_unittest.cc:45: #if defined(OS_CHROMEOS)
On 2016/06/21 22:17:53, James Cook wrote:
> On 2016/06/21 19:52:20, tdanderson wrote:
> > On 2016/06/16 16:08:56, James Cook wrote:
> > > Do you have to rework the #ifdefs this way? (Because of the
> parameterization?)
> > 
> > > If not, it might be useful to keep the tests compiling on Windows, even if
> > they
> > > are disabled.
> > 
> > To the best of my understanding this is required in order to get around the
> > parameterization. Though if I am missing something and you know of another
> way,
> > please let me know. (I tried WRAPPED_INSTANTIATE_TEST_CASE_P but without any
> > luck.)
> 
> Huh, it's weird that WRAPPED_INSTANTIATE_TEST_CASE_P doesn't work. That seems
> like exactly what you need:
>
https://cs-staging.chromium.org/chromium/src/net/cert/cert_verify_proc_unitte...
> 
> I wonder if it's because of the typedef / using.
> 
> Maybe give it one more try, and if it still doesn't work, just #ifdef it all
out
> like you're doing now.

I gave it another try without any success. I'll leave things as-is.

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/test/ash_md_...
File ash/test/ash_md_test_base.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/20001/ash/test/ash_md_...
ash/test/ash_md_test_base.cc:11: : md_maximized_window_height_offset_(0) {
On 2016/06/21 22:17:54, James Cook wrote:
> On 2016/06/21 19:52:21, tdanderson wrote:
> > On 2016/06/16 16:08:56, James Cook wrote:
> > > nit: init in header
> > 
> > Can you clarify what you mean here? Inlining a ctor looks to be forbidden by
> our
> > style guide, and assigning a value of 0 on line 34 seems a bit strange to
do.
> 
> I mean "int md_maximized_window_height_offset_ = 0;" in the header. Given that
> there is no complex initialization going on here, initializing in the header
> puts the default value closer to the declaration and makes it easier to see
that
> all POD members are initialized.

Done.

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/dip_unittest.cc
File ash/dip_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/dip_unittest...
ash/dip_unittest.cc:37: ,
On 2016/06/21 22:17:54, James Cook wrote:
> see previous comment

Done.

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
File ash/wm/window_state_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
ash/wm/window_state_unittest.cc:331: TEST_P(WindowStateTest,
RestoredWindowBoundsShrink) {
On 2016/06/21 22:17:54, James Cook wrote:
> On 2016/06/21 19:52:21, tdanderson wrote:
> > This test fails on CrOS with GetParam() == 2 (MATERIAL_EXPERIMENTAL) since
the
> > |work_area| bounds on line 335 are incorrectly being reported as 0,0 800x553
> > (should be the same as the maximized window bounds, which are 0,0 800x552).
> > 
> > Assuming this was due to stale display info in between test runs, in patch
set
> 2
> > I added UpdateDisplay("0+0-600x900") to the top of this test. This causes
the
> > test to pass on CrOS, but it fails on the Windows bots (again for the same
> > reason that the work area is incorrectly reported as 0,0 800x553).
> > 
> > Do you have any insight into how best to proceed with this? Looking at
> > DisplayManagerTestApi::UpdateDisplay(), I see that there is some code
> commented
> > out for OS_WIN marked with a TODO(msw). Unless you have any ideas offhand, I
> > plan on pinging msw@ to ask about that code.
> 
> Hmm. I'm not sure what you mean by "stale display info". Each individual test
> should construct a new AshTestBase and AshTestHelper. It's conceivable that
> there are static/global variables that are set that persist between tests.

I think that is what's happening, but I am unsuccessful in finding exactly what
is persisting. Calling UpdateDisplay() seems to refresh this state, as does the
call to window_state->Maximize() in WindowStateTest.AutoManage (line 287); the
AutoManage state does have its work area adjusted for material design on line
292.

> 
> It seems kinda weird that if you make the screen 900 pixels tall via
0+0-600x900
> that windows sees a work area ~550 pixels tall.
> 
> Does this repro for you locally on cros?  If not, oshima knows how to exactly
> repro the runtime environment from the bots on a local machine. I approximate
it
> with xvfb-run -s "-screen 0 1280x800x24" out/Default/ash_unittests

Yes, it reproduces for me locally as well.

> 
> I would ping msw too.

I have pinged him and he is going to take a look at this thread.

In the meanwhile, in the next patch set I've re-added the UpdateDisplay() calls
and removed the !defined(OS_WIN) in DisplayManagerTestApi::UpdateDisplay() to
see if a) the tests pass on Windows, and b) if it causes any test hangs on
Windows.

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
ash/wm/window_state_unittest.cc:377: // Exiting fullscreen will update the
maximized widnow to the work area.
On 2016/06/21 22:17:54, James Cook wrote:
> nit: while you're here, widnow -> window

Done.

[msw, 2016-06-22 17:06:07]

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

[msw, 2016-06-22 17:06:08]

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
File ash/wm/window_state_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
ash/wm/window_state_unittest.cc:331: TEST_P(WindowStateTest,
RestoredWindowBoundsShrink) {
On 2016/06/22 16:51:55, tdanderson wrote:
> I have pinged him and he is going to take a look at this thread.

Sorry, but I have little to add here. I excluded that code on Windows when
bringing up ash_unittests, simply because I empirically found that to be the
source of some hanging tests. Issue 247916 may be related (maybe ping
oshima/scottmg?). It *might* make sense to add an UpdateDisplay call and bail
early |if (!SupportsHostWindowResize())| like other fixtures in this suite, that
seems to be the popular way of sidestepping this Windows defect.

[tdanderson, 2016-06-22 19:15:46]

James, this is ready for another look.

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
File ash/wm/window_state_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
ash/wm/window_state_unittest.cc:331: TEST_P(WindowStateTest,
RestoredWindowBoundsShrink) {
On 2016/06/22 17:06:08, msw wrote:
> On 2016/06/22 16:51:55, tdanderson wrote:
> > I have pinged him and he is going to take a look at this thread.
> 
> Sorry, but I have little to add here. I excluded that code on Windows when
> bringing up ash_unittests, simply because I empirically found that to be the
> source of some hanging tests. Issue 247916 may be related (maybe ping
> oshima/scottmg?). It *might* make sense to add an UpdateDisplay call and bail
> early |if (!SupportsHostWindowResize())| like other fixtures in this suite,
that
> seems to be the popular way of sidestepping this Windows defect.

Thanks for the comments, I have used SupportsHostWindowResize() in the next
patch set.

msw@, as a side note, it looks like removing the !defined(OS_WIN) guard in
DisplayManagerTestApi::UpdateDisplay() did not cause any problems in the
trybots, so we should be good to remove that. I'd rather do it in a separate CL,
though, in case it needs to be reverted (in which case I don't want my Ash test
changes to be reverted).

[msw, 2016-06-22 19:26:08]

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
File ash/wm/window_state_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
ash/wm/window_state_unittest.cc:331: TEST_P(WindowStateTest,
RestoredWindowBoundsShrink) {
On 2016/06/22 19:15:46, tdanderson wrote:
> On 2016/06/22 17:06:08, msw wrote:
> > On 2016/06/22 16:51:55, tdanderson wrote:
> > > I have pinged him and he is going to take a look at this thread.
> > 
> > Sorry, but I have little to add here. I excluded that code on Windows when
> > bringing up ash_unittests, simply because I empirically found that to be the
> > source of some hanging tests. Issue 247916 may be related (maybe ping
> > oshima/scottmg?). It *might* make sense to add an UpdateDisplay call and
bail
> > early |if (!SupportsHostWindowResize())| like other fixtures in this suite,
> that
> > seems to be the popular way of sidestepping this Windows defect.
> 
> Thanks for the comments, I have used SupportsHostWindowResize() in the next
> patch set.
> 
> msw@, as a side note, it looks like removing the !defined(OS_WIN) guard in
> DisplayManagerTestApi::UpdateDisplay() did not cause any problems in the
> trybots, so we should be good to remove that. I'd rather do it in a separate
CL,
> though, in case it needs to be reverted (in which case I don't want my Ash
test
> changes to be reverted).

Great to hear! Lmk if you'd like me to follow up with that.

[tdanderson, 2016-06-22 19:30:08]

On 2016/06/22 19:26:08, msw wrote:
>
https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
> File ash/wm/window_state_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/2067223003/diff/60001/ash/wm/window_st...
> ash/wm/window_state_unittest.cc:331: TEST_P(WindowStateTest,
> RestoredWindowBoundsShrink) {
> On 2016/06/22 19:15:46, tdanderson wrote:
> > On 2016/06/22 17:06:08, msw wrote:
> > > On 2016/06/22 16:51:55, tdanderson wrote:
> > > > I have pinged him and he is going to take a look at this thread.
> > > 
> > > Sorry, but I have little to add here. I excluded that code on Windows when
> > > bringing up ash_unittests, simply because I empirically found that to be
the
> > > source of some hanging tests. Issue 247916 may be related (maybe ping
> > > oshima/scottmg?). It *might* make sense to add an UpdateDisplay call and
> bail
> > > early |if (!SupportsHostWindowResize())| like other fixtures in this
suite,
> > that
> > > seems to be the popular way of sidestepping this Windows defect.
> > 
> > Thanks for the comments, I have used SupportsHostWindowResize() in the next
> > patch set.
> > 
> > msw@, as a side note, it looks like removing the !defined(OS_WIN) guard in
> > DisplayManagerTestApi::UpdateDisplay() did not cause any problems in the
> > trybots, so we should be good to remove that. I'd rather do it in a separate
> CL,
> > though, in case it needs to be reverted (in which case I don't want my Ash
> test
> > changes to be reverted).
> 
> Great to hear! Lmk if you'd like me to follow up with that.

I've broken that change out into https://codereview.chromium.org/2089823003/
Let's follow up there.

[James Cook, 2016-06-22 19:41:37]

LGTM. In the future can you upload the rebase as a separate patch? It makes it
easier for me to review incremental changes.

https://chromiumcodereview.appspot.com/2067223003/diff/100001/ash/dip_unittes...
File ash/dip_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/100001/ash/dip_unittes...
ash/dip_unittest.cc:34: // The prefix has intentionally been left blank since
there is only one
optional: Feel free to remove these comments now that you have the inline one
below, and/or reword the inline one to something slightly longer like "no prefix
due to only one parameterization".

[tdanderson, 2016-06-22 20:31:33]

Description was changed from

==========
[WIP] Parameterize Ash unittests to pass with material design enabled

WIP, not for formal review.

BUG=617296
TEST=ash_unittests
==========

to

==========
Parameterize Ash unittests to pass with material design enabled

Parameterize test fixtures in ash_unittests
to run with the three modes used in Ash
material design (non-material, material stable,
and material experimental); see the runtime flag
--ash-md.

Also introduce the AshMdTestBase class to aid
in modifying test expectations across the
parameterized test runs.

BUG=617296
TEST=ash_unittests
==========

[tdanderson, 2016-06-22 20:57:27]

https://chromiumcodereview.appspot.com/2067223003/diff/100001/ash/dip_unittes...
File ash/dip_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/100001/ash/dip_unittes...
ash/dip_unittest.cc:34: // The prefix has intentionally been left blank since
there is only one
On 2016/06/22 19:41:36, James Cook wrote:
> optional: Feel free to remove these comments now that you have the inline one
> below, and/or reword the inline one to something slightly longer like "no
prefix
> due to only one parameterization".

Done.

https://chromiumcodereview.appspot.com/2067223003/diff/100001/ash/wm/system_m...
File ash/wm/system_modal_container_layout_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/2067223003/diff/100001/ash/wm/system_m...
ash/wm/system_modal_container_layout_manager_unittest.cc:773: if
(!SupportsMultipleDisplays())
moved this to https://codereview.chromium.org/2089823003/ where it rightfully
belongs.

[tdanderson, 2016-06-22 20:57:35]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2016-06-22 20:57:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2067223003/#ps120001 (title: "for
landing")

[commit-bot: I haz the power, 2016-06-22 20:58:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2067223003/120001

[commit-bot: I haz the power, 2016-06-22 21:53:19]

Description was changed from

==========
Parameterize Ash unittests to pass with material design enabled

Parameterize test fixtures in ash_unittests
to run with the three modes used in Ash
material design (non-material, material stable,
and material experimental); see the runtime flag
--ash-md.

Also introduce the AshMdTestBase class to aid
in modifying test expectations across the
parameterized test runs.

BUG=617296
TEST=ash_unittests
==========

to

==========
Parameterize Ash unittests to pass with material design enabled

Parameterize test fixtures in ash_unittests
to run with the three modes used in Ash
material design (non-material, material stable,
and material experimental); see the runtime flag
--ash-md.

Also introduce the AshMdTestBase class to aid
in modifying test expectations across the
parameterized test runs.

BUG=617296
TEST=ash_unittests
==========

[commit-bot: I haz the power, 2016-06-22 21:53:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-22 21:56:49]

Description was changed from

==========
Parameterize Ash unittests to pass with material design enabled

Parameterize test fixtures in ash_unittests
to run with the three modes used in Ash
material design (non-material, material stable,
and material experimental); see the runtime flag
--ash-md.

Also introduce the AshMdTestBase class to aid
in modifying test expectations across the
parameterized test runs.

BUG=617296
TEST=ash_unittests
==========

to

==========
Parameterize Ash unittests to pass with material design enabled

Parameterize test fixtures in ash_unittests
to run with the three modes used in Ash
material design (non-material, material stable,
and material experimental); see the runtime flag
--ash-md.

Also introduce the AshMdTestBase class to aid
in modifying test expectations across the
parameterized test runs.

BUG=617296
TEST=ash_unittests

Committed: https://crrev.com/0b7905b2457ab5222a853823b3ba817d4397f7a5
Cr-Commit-Position: refs/heads/master@{#401420}
==========

[commit-bot: I haz the power, 2016-06-22 21:56:50]

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