Merge lp:~kdub/mir/fix-1556045 into lp:mir

[Kevin DuBois]

fix: LP #1556045 which was blocking CI. problem was in the test fixture.

fix: LP #1556045 which was blocking CI. problem was in the test fixture.

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3462
https://mir-jenkins.ubuntu.com/job/mir-ci/840/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/851
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/888
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/879
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/879
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/861/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/861/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/861/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/861/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/861/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/840/rebuild

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

LGTM

review: Approve

[Alan Griffiths (alan-griffiths)]

+ testing::NiceMock<MockVisibilityCallback>& mock_callback)

Could use the interface?

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

=== modified file 'tests/acceptance-tests/test_client_surface_visibility.cpp'
--- tests/acceptance-tests/test_client_surface_visibility.cpp	2016-04-11 16:56:16 +0000
+++ tests/acceptance-tests/test_client_surface_visibility.cpp	2016-04-14 19:51:14 +0000
@@ -45,11 +45,8 @@
 class StoringShell : public msh::ShellWrapper
 {
 public:
-    StoringShell(
-        std::shared_ptr<msh::Shell> const& wrapped,
-        std::shared_ptr<msh::SurfaceStack> const surface_stack) :
-        msh::ShellWrapper{wrapped},
-        surface_stack{surface_stack}
+    StoringShell(std::shared_ptr<msh::Shell> const& wrapped) :
+        msh::ShellWrapper{wrapped}
     {}
 
     mf::SurfaceId create_surface(
@@ -71,12 +68,10 @@
 
     void raise(int index)
     {
-        surface_stack->raise(surface(index));
+        wrapped->raise_surface(nullptr, surface(index), 0);
     }
 
-    using msh::ShellWrapper::raise;
 private:
-    std::shared_ptr<msh::SurfaceStack> const surface_stack;
     std::vector<std::weak_ptr<ms::Surface>> surfaces;
 
 };
@@ -94,56 +89,103 @@
     if (mir_surface_event_get_attribute(sev) != mir_surface_attrib_visibility)
         return;
 
-    auto const mock_visibility_callback =
+    auto const mock_callback =
         reinterpret_cast<testing::NiceMock<MockVisibilityCallback>*>(ctx);
-    mock_visibility_callback->handle(
+    mock_callback->handle(
         surface,
         static_cast<MirSurfaceVisibility>(mir_surface_event_get_attribute_value(sev)));
 }
 
-struct MirSurfaceVisibilityEvent : mtf::ConnectedClientWithASurface
-{
-
+MirSurface* create_surface(MirConnection* connection, geom::Size size,
+    testing::NiceMock<MockVisibilityCallback>& mock_callback)
+{
+    auto const spec = mir_connection_create_spec_for_normal_surface(
+        connection, size.width.as_int(), size.height.as_int(), mir_pixel_format_bgr_888);
+    mir_surface_spec_set_name(spec, "ConnectedClientWithASurfaceFixtureSurface");
+    mir_surface_spec_set_buffer_usage(spec, mir_buffer_usage_hardware);
+    mir_surface_spec_set_event_handler(spec, &event_callback, &mock_callback);
+    auto surface = mir_surface_create_sync(spec);
+    mir_surface_spec_release(spec);
+    return surface;
+}
+
+struct Surface
+{
+    Surface(MirConnection* connection, geom::Size size) :
+        surface(create_surface(connection, size, callback))
+    {
+        wait_for_visible();
+    }
+
+    ~Surface()
+    {
+        mir_surface_release_sync(surface);
+    }
+
+    void expect_surface_visibility_event_after(
+        MirSurfaceVisibility visibility,
+        std::function<void()> const& action)
+    {
+        using namespace testing;
+
+        mt::Signal event_received;
+
+        Mock::VerifyAndClearExpectations(&callback);
+
+        EXPECT_CALL(callback, handle(surface, visibility))
+            .WillOnce(DoAll(Invoke([&visibility](MirSurface *s, MirSurfaceVisibility)
+                {
+                    EXPECT_EQ(visibility, mir_surface_get_visibility(s));
+                }), mt::WakeUp(&event_received)));
+
+        action();
+
+        event_received.wait_for(std::chrono::seconds{2});
+
+        Mock::VerifyAndClearExpectations(&callback);
+    }
+
+private:
+    void wait_for_visible()
+    {
+        expect_surface_visibility_event_after(
+            mir_surface_visibility_exposed,
+            [this]
+            {
+                mir_buffer_stream_swap_buffers_sync(mir_surface_get_buffer_stream(surface));
+            });
+    }
+
+    testing::NiceMock<MockVisibilityCallback> callback;
+    MirSurface* surface;
+};
+
+struct MirSurfaceVisibilityEvent : mtf::ConnectedClientHeadlessServer
+{
     void SetUp() override
     {
         server.wrap_shell([&](std::shared_ptr<msh::Shell> const& wrapped)
             {
-                auto const result = std::make_shared<StoringShell>(wrapped, server.the_surface_stack());
+                auto const result = std::make_shared<StoringShell>(wrapped);
                 shell = result;
                 return result;
             });
 
-        mtf::ConnectedClientWithASurface::SetUp();
-
-        mir_surface_set_event_handler(surface, &event_callback, &mock_visibility_callback);
-
-        // Swap enough buffers to ensure compositor threads are into run loop
-        for (auto i = 0; i != 11; ++i)
-            mir_buffer_stream_swap_buffers_sync(mir_surface_get_buffer_stream(surface));
+        mtf::ConnectedClientHeadlessServer::SetUp();
+        surface = std::make_unique<Surface>(connection, small_size);
     }
 
     void TearDown() override
     {
-        // Don't call ConnectedClientWithASurface::TearDown() - the sequence matters
-        mir_surface_release_sync(surface);
-        if (second_surface)
-            mir_surface_release_sync(second_surface);
-
+        surface.reset();
+        second_surface.reset();
         mtf::ConnectedClientHeadlessServer::TearDown();
     }
 
     void create_larger_surface_on_top()
     {
-        auto spec = mir_connection_create_spec_for_normal_surface(connection, 800, 600, mir_pixel_format_bgr_888);
-
-        second_surface = mir_surface_create_sync(spec);
-        ASSERT_TRUE(mir_surface_is_valid(second_surface));
-    
-        mir_surface_spec_release(spec);
-
+        second_surface = std::make_unique<Surface>(connection, large_size);
         shell.lock()->raise(1);
-
-        mir_buffer_stream_swap_buffers_sync(mir_surface_get_buffer_stream(second_surface));
     }
 
     std::shared_ptr<ms::Surface> server_surface(size_t index)
@@ -170,27 +212,13 @@
         MirSurfaceVisibility visibility,
         std::function<void()> const& action)
     {
-        using namespace testing;
-
-        mt::Signal event_received;
-
-        Mock::VerifyAndClearExpectations(&mock_visibility_callback);
-
-        EXPECT_CALL(mock_visibility_callback, handle(surface, visibility))
-            .WillOnce(DoAll(Invoke([&visibility](MirSurface *s, MirSurfaceVisibility)
-                {
-                    EXPECT_EQ(visibility, mir_surface_get_visibility(s));
-                }), mt::WakeUp(&event_received)));
-
-        action();
-
-        event_received.wait_for(std::chrono::seconds{2});
-
-        Mock::VerifyAndClearExpectations(&mock_visibility_callback);
+        surface->expect_surface_visibility_event_after(visibility, action);
     }
 
-    MirSurface* second_surface = nullptr;
-    testing::NiceMock<MockVisibilityCallback> mock_visibility_callback;
+    geom::Size const small_size {640, 480};
+    geom::Size const large_size {800, 600};
+    std::unique_ptr<Surface> surface;
+    std::unique_ptr<Surface> second_surface;
     std::weak_ptr<StoringShell> shell;
 };