Parameterize Ash unittests to pass with material design enabled

ash/wm/window_state_unittest.cc

Index: ash/wm/window_state_unittest.cc
diff --git a/ash/wm/window_state_unittest.cc b/ash/wm/window_state_unittest.cc
index 21b2431bb94405595c2c90661af02b7917380c6f..5488c7e3e20d87e5ac1d09749a12facf87eea9b5 100644
--- a/ash/wm/window_state_unittest.cc
+++ b/ash/wm/window_state_unittest.cc
@@ -6,10 +6,11 @@
 
 #include <utility>
 
+#include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/wm/window_state.h"
 #include "ash/common/wm/wm_event.h"
 #include "ash/screen_util.h"
-#include "ash/test/ash_test_base.h"
+#include "ash/test/ash_md_test_base.h"
 #include "ash/wm/window_state_aura.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/test/test_window_delegate.h"
@@ -49,11 +50,20 @@ class AlwaysMaximizeTestState : public WindowState::State {
 
 }  // namespace
 
-typedef test::AshTestBase WindowStateTest;
+using WindowStateTest = test::AshMDTestBase;
+
+// Note: First argument is optional and intentionally left blank.
+// (it's a prefix for the generated test cases)
+INSTANTIATE_TEST_CASE_P(
+    ,
+    WindowStateTest,
+    testing::Values(MaterialDesignController::NON_MATERIAL,
+                    MaterialDesignController::MATERIAL_NORMAL,
+                    MaterialDesignController::MATERIAL_EXPERIMENTAL));
 
 // Test that a window gets properly snapped to the display's edges in a
 // multi monitor environment.
-TEST_F(WindowStateTest, SnapWindowBasic) {
+TEST_P(WindowStateTest, SnapWindowBasic) {
   if (!SupportsMultipleDisplays())
     return;
 
@@ -100,7 +110,7 @@ TEST_F(WindowStateTest, SnapWindowBasic) {
 
 // Test how the minimum and maximum size specified by the aura::WindowDelegate
 // affect snapping.
-TEST_F(WindowStateTest, SnapWindowMinimumSize) {
+TEST_P(WindowStateTest, SnapWindowMinimumSize) {
   if (!SupportsHostWindowResize())
     return;
 
@@ -136,7 +146,7 @@ TEST_F(WindowStateTest, SnapWindowMinimumSize) {
 }
 
 // Test that the minimum size specified by aura::WindowDelegate gets respected.
-TEST_F(WindowStateTest, TestRespectMinimumSize) {
+TEST_P(WindowStateTest, TestRespectMinimumSize) {
   if (!SupportsHostWindowResize())
     return;
 
@@ -166,7 +176,7 @@ TEST_F(WindowStateTest, TestRespectMinimumSize) {
 
 // Test that the minimum window size specified by aura::WindowDelegate does not
 // exceed the screen size.
-TEST_F(WindowStateTest, TestIgnoreTooBigMinimumSize) {
+TEST_P(WindowStateTest, TestIgnoreTooBigMinimumSize) {
   if (!SupportsHostWindowResize())
     return;
 
@@ -197,7 +207,7 @@ TEST_F(WindowStateTest, TestIgnoreTooBigMinimumSize) {
 }
 
 // Test that setting the bounds of a snapped window keeps its snapped.
-TEST_F(WindowStateTest, SnapWindowSetBounds) {
+TEST_P(WindowStateTest, SnapWindowSetBounds) {
   if (!SupportsHostWindowResize())
     return;
 
@@ -225,7 +235,7 @@ TEST_F(WindowStateTest, SnapWindowSetBounds) {
 }
 
 // Test that snapping left/right preserves the restore bounds.
-TEST_F(WindowStateTest, RestoreBounds) {
+TEST_P(WindowStateTest, RestoreBounds) {
   std::unique_ptr<aura::Window> window(
       CreateTestWindowInShellWithBounds(gfx::Rect(100, 100, 100, 100)));
   WindowState* window_state = GetWindowState(window.get());
@@ -266,7 +276,7 @@ TEST_F(WindowStateTest, RestoreBounds) {
 
 // Test that maximizing an auto managed window, then snapping it puts the window
 // at the snapped bounds and not at the auto-managed (centered) bounds.
-TEST_F(WindowStateTest, AutoManaged) {
+TEST_P(WindowStateTest, AutoManaged) {
   std::unique_ptr<aura::Window> window(CreateTestWindowInShellWithId(0));
   WindowState* window_state = GetWindowState(window.get());
   window_state->set_window_position_managed(true);
@@ -293,7 +303,7 @@ TEST_F(WindowStateTest, AutoManaged) {
 }
 
 // Test that the replacement of a State object works as expected.
-TEST_F(WindowStateTest, SimpleStateSwap) {
+TEST_P(WindowStateTest, SimpleStateSwap) {
   std::unique_ptr<aura::Window> window(CreateTestWindowInShellWithId(0));
   WindowState* window_state = GetWindowState(window.get());
   EXPECT_FALSE(window_state->IsMaximized());
@@ -304,7 +314,7 @@ TEST_F(WindowStateTest, SimpleStateSwap) {
 
 // Test that the replacement of a state object, following a restore with the
 // original one restores the window to its original state.
-TEST_F(WindowStateTest, StateSwapRestore) {
+TEST_P(WindowStateTest, StateSwapRestore) {
   std::unique_ptr<aura::Window> window(CreateTestWindowInShellWithId(0));
   WindowState* window_state = GetWindowState(window.get());
   EXPECT_FALSE(window_state->IsMaximized());
@@ -318,7 +328,7 @@ TEST_F(WindowStateTest, StateSwapRestore) {
 
 // Tests that a window that had same bounds as the work area shrinks after the
 // window is maximized and then restored.
-TEST_F(WindowStateTest, RestoredWindowBoundsShrink) {
+TEST_P(WindowStateTest, RestoredWindowBoundsShrink) {

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

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]

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.

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

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

[tdanderson, 2016/06/22 16:51:55]

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.
Yes, it reproduces for me locally as well.
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.

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

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]

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]

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

   std::unique_ptr<aura::Window> window(CreateTestWindowInShellWithId(0));
   WindowState* window_state = GetWindowState(window.get());
   EXPECT_FALSE(window_state->IsMaximized());
@@ -336,16 +346,18 @@ TEST_F(WindowStateTest, RestoredWindowBoundsShrink) {
   EXPECT_TRUE(work_area.Contains(window->bounds()));
 }
 
-TEST_F(WindowStateTest, DoNotResizeMaximizedWindowInFullscreen) {
+TEST_P(WindowStateTest, DoNotResizeMaximizedWindowInFullscreen) {
   if (!SupportsHostWindowResize())
     return;
+  const int height_offset = GetMdMaximizedWindowHeightOffset();
 
   std::unique_ptr<aura::Window> maximized(CreateTestWindowInShellWithId(0));
   std::unique_ptr<aura::Window> fullscreen(CreateTestWindowInShellWithId(1));
   WindowState* maximized_state = GetWindowState(maximized.get());
   maximized_state->Maximize();
   ASSERT_TRUE(maximized_state->IsMaximized());
-  EXPECT_EQ("0,0 800x553", maximized->GetBoundsInScreen().ToString());
+  EXPECT_EQ(gfx::Rect(0, 0, 800, 553 + height_offset).ToString(),
+            maximized->GetBoundsInScreen().ToString());
 
   // Entering fullscreen mode will not update the maximized window's size
   // under fullscreen.
@@ -354,15 +366,17 @@ TEST_F(WindowStateTest, DoNotResizeMaximizedWindowInFullscreen) {
   fullscreen_state->OnWMEvent(&fullscreen_event);
   ASSERT_TRUE(fullscreen_state->IsFullscreen());
   ASSERT_TRUE(maximized_state->IsMaximized());
-  EXPECT_EQ("0,0 800x553", maximized->GetBoundsInScreen().ToString());
+  EXPECT_EQ(gfx::Rect(0, 0, 800, 553 + height_offset).ToString(),
+            maximized->GetBoundsInScreen().ToString());
 
   // Updating display size will update the maximum window size.
   UpdateDisplay("900x700");
   EXPECT_EQ("0,0 900x700", maximized->GetBoundsInScreen().ToString());
   fullscreen.reset();
 
-  // Exitting fullscreen will update the maximized widnow to the work area.
-  EXPECT_EQ("0,0 900x653", maximized->GetBoundsInScreen().ToString());
+  // Exiting fullscreen will update the maximized widnow to the work area.

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

nit: while you're here, widnow -> window

[tdanderson, 2016/06/22 16:51:55]

Done.

+  EXPECT_EQ(gfx::Rect(0, 0, 900, 653 + height_offset).ToString(),
+            maximized->GetBoundsInScreen().ToString());
 }
 
 // TODO(skuhne): Add more unit test to verify the correctness for the restore