Merge lp:~gerboland/miral/fix-windowmanager-tests into lp:miral

[Gerry Boland]

[miral-qt] Fix failing tests due to lack of event loop, and fix mocking the Window size so can enable its test

[Gerry Boland (gerboland)]

I could add functions to wrap each notifier.* method with flush() call, but wasn't sure the test readability would be enhanced. Second opinion desired.

[Alan Griffiths (alan-griffiths)]

+// windowSpec.of_size(toMirSize(size)); // useless, Window/Surface has the size actually used

useless

[Alan Griffiths (alan-griffiths)]

> I could add functions to wrap each notifier.* method with flush() call, but
> wasn't sure the test readability would be enhanced. Second opinion desired.

Well, I'd be tempted by a bit more a restructuring of the tests:

All the tests start with:

    WindowModelNotifier notifier;
    WindowModel model(&notifier, nullptr); // no need for controller in this testcase

I don't see why these can't be moved into the fixture.

Then the fixture can have:

    void processEvents(std::function<void>(WindowModelNotifier& notifier) const& eventSource)
    {
        eventSource(notifier);
        qtApp->sendPostedEvents();
    }

and the tests can have:

    processEvents([&](WindowModelNotifier& notifier)
        {
            notifier.addWindow(mirWindowInfo1);
            notifier.addWindow(mirWindowInfo2);
        });

[Alan Griffiths (alan-griffiths)]

If you want to land this and (optionally) rework the tests later then I'm fine with that.

review: Approve

[Gerry Boland (gerboland)]

Rework more significant, so I'd prefer doing it separately

=== modified file 'miral-qt/tests/modules/WindowManager/windowmodel_test.cpp'
--- miral-qt/tests/modules/WindowManager/windowmodel_test.cpp	2016-09-07 14:11:13 +0000
+++ miral-qt/tests/modules/WindowManager/windowmodel_test.cpp	2016-09-08 23:24:56 +0000
@@ -34,10 +34,19 @@
 
 namespace ms = mir::scene;
 namespace mg = mir::graphics;
-using StubSurface = mir::test::doubles::StubSurface;
 using StubSession = mir::test::doubles::StubSession;
 using namespace testing;
 
+struct SizedStubSurface : public mir::test::doubles::StubSurface
+{
+    mir::geometry::Size size() const override { return toMirSize(m_size); }
+
+    void setSize(QSize size) { m_size = size; }
+
+private:
+    QSize m_size;
+};
+
 
 class WindowModelTest : public ::testing::Test
 {
@@ -51,16 +60,18 @@
     miral::WindowInfo createMirALWindowInfo(QPoint position = {160, 320}, QSize size = {100, 200})
     {
         const miral::Application app{stubSession};
+        stubSurface->setSize(size);
         const miral::Window window{app, stubSurface};
 
         ms::SurfaceCreationParameters windowSpec;
-        windowSpec.of_size(toMirSize(size));
+//        windowSpec.of_size(toMirSize(size)); // useless, Window/Surface has the size actually used
         windowSpec.of_position(toMirPoint(position));
         return miral::WindowInfo{window, windowSpec};
     }
 
     MirSurface *getMirSurfaceFromModel(const WindowModel &model, int index)
     {
+        flushEvents();
         return model.data(model.index(index, 0), WindowModel::SurfaceRole).value<MirSurface*>();
     }
 
@@ -69,8 +80,26 @@
         return getMirSurfaceFromModel(model, index)->window();
     }
 
+    void SetUp() override
+    {
+        int argc = 0;
+        char* argv[0];
+        qtApp = new QCoreApplication(argc, argv); // needed for event loop
+    }
+
+    void TearDown() override
+    {
+        delete qtApp;
+    }
+
+    void flushEvents()
+    {
+        qtApp->sendPostedEvents();
+    }
+
     const std::shared_ptr<StubSession> stubSession{std::make_shared<StubSession>()};
-    const std::shared_ptr<StubSurface> stubSurface{std::make_shared<StubSurface>()};
+    const std::shared_ptr<SizedStubSurface> stubSurface{std::make_shared<SizedStubSurface>()};
+    QCoreApplication *qtApp;
 };
 
 /*
@@ -85,6 +114,7 @@
     auto mirWindowInfo = createMirALWindowInfo();
 
     notifier.addWindow(mirWindowInfo);
+    flushEvents();
 
     EXPECT_EQ(1, model.count());
 }
@@ -103,6 +133,7 @@
     QSignalSpy spyCountChanged(&model, SIGNAL(countChanged()));
 
     notifier.addWindow(mirWindowInfo);
+    flushEvents();
 
     EXPECT_EQ(1, spyCountChanged.count());
 }
@@ -119,6 +150,7 @@
     auto mirWindowInfo = createMirALWindowInfo();
 
     notifier.addWindow(mirWindowInfo);
+    flushEvents();
 
     auto miralWindow = getMirALWindowFromModel(model, 0);
     EXPECT_EQ(mirWindowInfo.window(), miralWindow);
@@ -138,6 +170,7 @@
 
     // Test removing the window
     notifier.removeWindow(mirWindowInfo);
+    flushEvents();
 
     EXPECT_EQ(0, model.count());
 }
@@ -153,11 +186,13 @@
 
     auto mirWindowInfo = createMirALWindowInfo();
     notifier.addWindow(mirWindowInfo);
+    flushEvents();
 
     // Test removing the window
     QSignalSpy spyCountChanged(&model, SIGNAL(countChanged()));
 
     notifier.removeWindow(mirWindowInfo);
+    flushEvents();
 
     EXPECT_EQ(1, spyCountChanged.count());
 }
@@ -176,6 +211,7 @@
 
     notifier.addWindow(mirWindowInfo1);
     notifier.addWindow(mirWindowInfo2);
+    flushEvents();
 
     ASSERT_EQ(2, model.count());
     auto miralWindow1 = getMirALWindowFromModel(model, 0);
@@ -200,6 +236,7 @@
 
     // Remove second window
     notifier.removeWindow(mirWindowInfo2);
+    flushEvents();
 
     ASSERT_EQ(1, model.count());
     auto miralWindow = getMirALWindowFromModel(model, 0);
@@ -222,6 +259,7 @@
 
     // Remove first window
     notifier.removeWindow(mirWindowInfo1);
+    flushEvents();
 
     ASSERT_EQ(1, model.count());
     auto miralWindow = getMirALWindowFromModel(model, 0);
@@ -245,6 +283,7 @@
     notifier.removeWindow(mirWindowInfo1);
 
     notifier.addWindow(mirWindowInfo3);
+    flushEvents();
 
     ASSERT_EQ(2, model.count());
     auto miralWindow2 = getMirALWindowFromModel(model, 0);
@@ -270,6 +309,7 @@
     notifier.addWindow(mirWindowInfo3);
 
     notifier.removeWindow(mirWindowInfo2);
+    flushEvents();
 
     ASSERT_EQ(2, model.count());
     auto miralWindow1 = getMirALWindowFromModel(model, 0);
@@ -291,6 +331,7 @@
 
     // Raise first window
     notifier.raiseWindows({mirWindowInfo1.window()});
+    flushEvents();
 
     ASSERT_EQ(1, model.count());
     auto topWindow = getMirALWindowFromModel(model, 0);
@@ -312,6 +353,7 @@
 
     // Raise second window (currently on top)
     notifier.raiseWindows({mirWindowInfo2.window()});
+    flushEvents();
 
     // Check second window still on top
     ASSERT_EQ(2, model.count());
@@ -334,6 +376,7 @@
 
     // Raise first window (currently at bottom)
     notifier.raiseWindows({mirWindowInfo1.window()});
+    flushEvents();
 
     // Check first window now on top
     ASSERT_EQ(2, model.count());
@@ -368,6 +411,8 @@
     // 2:   Window1
     // 1:   Window2
     // 0:   Window3
+    flushEvents();
+
     ASSERT_EQ(3, model.count());
     auto topWindow = getMirALWindowFromModel(model, 2);
     EXPECT_EQ(mirWindowInfo1.window(), topWindow);
@@ -400,6 +445,8 @@
     // Model should now be like this:
     // 1:   Window1
     // 0:   Window2
+    flushEvents();
+
     ASSERT_EQ(2, model.count());
     auto topWindow = getMirALWindowFromModel(model, 1);
     EXPECT_EQ(mirWindowInfo1.window(), topWindow);
@@ -435,6 +482,8 @@
     // 2:   Window2
     // 1:   Window1
     // 0:   Window3
+    flushEvents();
+
     ASSERT_EQ(3, model.count());
     auto topWindow = getMirALWindowFromModel(model, 2);
     EXPECT_EQ(mirWindowInfo2.window(), topWindow);
@@ -456,6 +505,7 @@
 
     auto mirWindowInfo = createMirALWindowInfo(position);
     notifier.addWindow(mirWindowInfo);
+    flushEvents();
 
     auto surface = getMirSurfaceFromModel(model, 0);
     EXPECT_EQ(position, surface->position());
@@ -479,6 +529,7 @@
 
     // Move window, check new position set
     notifier.moveWindow(mirWindowInfo, toMirPoint(newPosition));
+    flushEvents();
 
     EXPECT_EQ(newPosition, surface->position());
 }
@@ -503,6 +554,7 @@
 
     // Move window, check new position set
     notifier.moveWindow(mirWindowInfo1, toMirPoint(newPosition));
+    flushEvents();
 
     EXPECT_EQ(newPosition, surface->position());
 }
@@ -526,6 +578,7 @@
 
     // Move window, check new position set
     notifier.moveWindow(mirWindowInfo1, toMirPoint(QPoint(350, 420)));
+    flushEvents();
 
     // Ensure other window untouched
     EXPECT_EQ(fixedPosition, surface->position());
@@ -543,6 +596,7 @@
 
     auto mirWindowInfo1 = createMirALWindowInfo(QPoint(), size);
     notifier.addWindow(mirWindowInfo1);
+    flushEvents();
 
     auto surface = getMirSurfaceFromModel(model, 0);
     EXPECT_EQ(size, surface->size());
@@ -563,8 +617,9 @@
 
     auto surface = getMirSurfaceFromModel(model, 0);
 
-    // Move window, check new position set
+    // Resize window, check new size set
     notifier.resizeWindow(mirWindowInfo1, toMirSize(newSize));
+    flushEvents();
 
     EXPECT_EQ(newSize, surface->size());
 }
@@ -586,8 +641,9 @@
 
     auto surface = getMirSurfaceFromModel(model, 0);
 
-    // Move window, check new position set
+    // Resize window, check new size set
     notifier.resizeWindow(mirWindowInfo1, toMirSize(newSize));
+    flushEvents();
 
     EXPECT_EQ(newSize, surface->size());
 }
@@ -595,23 +651,24 @@
 /*
  * Test: with 2 windows, ensure window resize does not impact other MirSurfaces
  */
-TEST_F(WindowModelTest, DISABLED_WindowResizeDoesNotTouchOtherMirSurfaces)
+TEST_F(WindowModelTest, WindowResizeDoesNotTouchOtherMirSurfaces)
 {
     WindowModelNotifier notifier;
     WindowModel model(&notifier, nullptr); // no need for controller in this testcase
 
-    QSize fixedPosition(300, 400);
+    QSize fixedSize(300, 400);
 
     auto mirWindowInfo1 = createMirALWindowInfo(QPoint(), QSize(100, 200));
-    auto mirWindowInfo2 = createMirALWindowInfo(QPoint(), fixedPosition);
+    auto mirWindowInfo2 = createMirALWindowInfo(QPoint(), fixedSize);
     notifier.addWindow(mirWindowInfo1);
     notifier.addWindow(mirWindowInfo2);
 
     auto surface = getMirSurfaceFromModel(model, 1);
 
-    // Move window
+    // Resize window
     notifier.resizeWindow(mirWindowInfo1, toMirSize(QSize(150, 220)));
+    flushEvents();
 
     // Ensure other window untouched
-    EXPECT_EQ(fixedPosition, surface->size());
+    EXPECT_EQ(fixedSize, surface->size());
 }