Parameterize Ash unittests to pass with material design enabled

ash/dip_unittest.cc

Index: ash/dip_unittest.cc
diff --git a/ash/dip_unittest.cc b/ash/dip_unittest.cc
index 048fa50d40db1c554eda0f9b71d9471bf7c94f1e..c69aecaf922e14ed63d2fd0fe0a4424e356f72b3 100644
--- a/ash/dip_unittest.cc
+++ b/ash/dip_unittest.cc
@@ -6,11 +6,12 @@
 #include <vector>
 
 #include "ash/ash_switches.h"
+#include "ash/common/material_design/material_design_controller.h"
 #include "ash/display/display_manager.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_widget.h"
 #include "ash/shell.h"
-#include "ash/test/ash_test_base.h"
+#include "ash/test/ash_md_test_base.h"
 #include "ash/wm/window_properties.h"
 #include "ash/wm/window_util.h"
 #include "base/command_line.h"
@@ -28,16 +29,22 @@
 
 namespace ash {
 
-typedef ash::test::AshTestBase DIPTest;
+typedef ash::test::AshMDTestBase DIPTest;

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

nit: using DIPTest =

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

Done.

+
+// Note: First argument is optional and intentionally left blank.

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

Why leave this out? Most other parameterized tests use it.

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

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.

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

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 */

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

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

+// (it's a prefix for the generated test cases)
+INSTANTIATE_TEST_CASE_P(
+    ,
+    DIPTest,
+    testing::Values(ash::MaterialDesignController::NON_MATERIAL,

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

nit: no ash::

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

Done.

+                    ash::MaterialDesignController::MATERIAL_NORMAL,
+                    ash::MaterialDesignController::MATERIAL_EXPERIMENTAL));
 
 // Test if the WM sets correct work area under different density.
-#if defined(OS_WIN) && !defined(USE_ASH)
 // TODO(msw): Broken on Windows. http://crbug.com/584038
-#define MAYBE_WorkArea DISABLED_WorkArea
-#else
-#define MAYBE_WorkArea WorkArea
-#endif
-TEST_F(DIPTest, MAYBE_WorkArea) {
+#if defined(OS_CHROMEOS)

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

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.

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

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.)

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

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.

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

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

+TEST_P(DIPTest, WorkArea) {
+  const int height_offset = GetMdMaximizedWindowHeightOffset();
   UpdateDisplay("1000x900*1.0f");
 
   aura::Window* root = Shell::GetPrimaryRootWindow();
@@ -46,8 +53,10 @@ TEST_F(DIPTest, MAYBE_WorkArea) {
 
   EXPECT_EQ("0,0 1000x900", display.bounds().ToString());
   gfx::Rect work_area = display.work_area();
-  EXPECT_EQ("0,0 1000x853", work_area.ToString());
-  EXPECT_EQ("0,0,47,0", display.bounds().InsetsFrom(work_area).ToString());
+  EXPECT_EQ(gfx::Rect(0, 0, 1000, 853 + height_offset).ToString(),
+            work_area.ToString());
+  EXPECT_EQ(gfx::Insets(0, 0, 47 - height_offset, 0).ToString(),
+            display.bounds().InsetsFrom(work_area).ToString());
 
   UpdateDisplay("2000x1800*2.0f");
   display::Screen* screen = display::Screen::GetScreen();
@@ -63,8 +72,10 @@ TEST_F(DIPTest, MAYBE_WorkArea) {
   // Aura and views coordinates are in DIP, so they their bounds do not change.
   EXPECT_EQ("0,0 1000x900", display_2x.bounds().ToString());
   work_area = display_2x.work_area();
-  EXPECT_EQ("0,0 1000x853", work_area.ToString());
-  EXPECT_EQ("0,0,47,0", display_2x.bounds().InsetsFrom(work_area).ToString());
+  EXPECT_EQ(gfx::Rect(0, 0, 1000, 853 + height_offset).ToString(),
+            work_area.ToString());
+  EXPECT_EQ(gfx::Insets(0, 0, 47 - height_offset, 0).ToString(),
+            display_2x.bounds().InsetsFrom(work_area).ToString());
 
   // Sanity check if the workarea's inset hight is same as
   // the shelf's height.
@@ -73,5 +84,6 @@ TEST_F(DIPTest, MAYBE_WorkArea) {
       display_2x.bounds().InsetsFrom(work_area).height(),
       shelf->shelf_widget()->GetNativeView()->layer()->bounds().height());
 }
+#endif  // defined(OS_CHROMEOS)
 
 }  // namespace ash