Merge lp:~robertcarr/mir/reduce-coupling-in-application-mediator-tests into lp:~mir-team/mir/trunk

[Robert Carr]

Reduce and code coupling in application mediator tests through usage of mock objects as opposed to a mix of stub and real objects.

Reduce duplication and coupling in application mediator tests through usage of mock objects rather than a mix of stubs and real objects (i.e. ApplicationSession).

Depends on the mock definition header (mock_session.h) introduced here:
https://code.launchpad.net/~robertcarr/mir/dedupe-mock-session/+merge/152287

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:475
http://jenkins.qa.ubuntu.com/job/mir-ci/18/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/18//console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/18//rebuild/?

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

The changes introduces mocks where only stubs are needed (i.e. no behavioral testing is going on). Examples:

273 +class StubbedSession : public testing::NiceMock<mtd::MockSession>
412 + : session_store{std::make_shared<mtd::MockSessionStore>()},
503 + : session_store{std::make_shared<mtd::MockSessionStore>()},

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

116 - * it under the terms of the GNU General Public License version 3 as
117 + * it under the terms of the GNU Lesser General Public License version 3 as

My understanding of Canonical policy is that executables are GPL, only libraries are LGPL.

review: Needs Fixing

[Robert Carr (robertcarr)]

Alan: Fixed license...I didn't get the memo when things changed and got confused by review comments :)

Alf: Replaced MockSessionStore (test_application_mediator*) and MockSession in test_application_mediator.cpp with stubs...also removed the NiceMock on the surface

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:478
http://jenkins.qa.ubuntu.com/job/mir-ci/30/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/30//console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/30//rebuild/?

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I think some licenses are still wrong.

What's going on with StubSessionStore? There appear to be two identical derived classes in separate test files - and no other uses. If the idea is to deduplicate two stubs that were accidentally the same, then why end up with three, one of which has behaviour that isn't used?

I don't see the need to replace perfectly good stub classes using NiceMock to adapt mock classes to that need.

review: Needs Fixing

[Robert Carr (robertcarr)]

I've fixed StubSessionStore to be shared between test_application_mediator_gbm and test_application_mediator_android. test_application_mediator is still using a mock session store due to the behavior testing on close_session.

Likewise test_application_mediator is still using a mock surface in the stub session (test_application_mediator::StubSession::get_surface) in an attempt to add some specification to the interface between surface and application mediator (which methods of surface may application mediator access?). Ultimately I think we will end up with another interface here, but I think it's a mild improvement.

[Robert Carr (robertcarr)]

Fixed license headers

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:480
http://jenkins.qa.ubuntu.com/job/mir-ci/46/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/47//console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/46//rebuild/?

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:481
http://jenkins.qa.ubuntu.com/job/mir-ci/48/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/49//console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/48//rebuild/?

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

LBTM

review: Approve

[Alexandros Frantzis (afrantzis)]

> Likewise test_application_mediator is still using a mock surface in the stub
> session (test_application_mediator::StubSession::get_surface) in an attempt to
> add some specification to the interface between surface and application
> mediator (which methods of surface may application mediator access?).

I don't understand how having a mock here will help, but I guess we can always
change it to a stub later as needed.

review: Approve

=== modified file 'include/mir_test_doubles/mock_session.h'
--- include/mir_test_doubles/mock_session.h	2013-03-07 21:35:30 +0000
+++ include/mir_test_doubles/mock_session.h	2013-03-12 00:14:21 +0000
@@ -1,16 +1,16 @@
 /*
  * Copyright © 2013 Canonical Ltd.
  *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 3 as
- * published by the Free Software Foundation.
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
  *
  * This program is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
- * You should have received a copy of the GNU General Public License
+ * You should have received a copy of the GNU Lesser General Public License
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
  * Authored by: Robert Carr <[e-mail]>

=== added file 'include/mir_test_doubles/mock_session_store.h'
--- include/mir_test_doubles/mock_session_store.h	1970-01-01 00:00:00 +0000
+++ include/mir_test_doubles/mock_session_store.h	2013-03-12 00:14:21 +0000
@@ -0,0 +1,48 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_TEST_DOUBLES_MOCK_SESSION_STORE_H_
+#define MIR_TEST_DOUBLES_MOCK_SESSION_STORE_H_
+
+#include "mir/shell/session_store.h"
+
+#include <gmock/gmock.h>
+
+namespace mir
+{
+namespace test
+{
+namespace doubles
+{
+
+struct MockSessionStore : public shell::SessionStore
+{
+    MOCK_METHOD1(open_session, std::shared_ptr<shell::Session>(std::string const&));
+    MOCK_METHOD1(close_session, void(std::shared_ptr<shell::Session> const&));
+
+    MOCK_METHOD2(tag_session_with_lightdm_id, void(std::shared_ptr<shell::Session> const&, int));
+    MOCK_METHOD1(focus_session_with_lightdm_id, void(int));
+
+    MOCK_METHOD0(shutdown, void());
+};
+
+}
+}
+} // namespace mir
+
+#endif // MIR_TEST_DOUBLES_MOCK_SESSION_STORE_H_

=== added file 'include/mir_test_doubles/mock_surface.h'
--- include/mir_test_doubles/mock_surface.h	1970-01-01 00:00:00 +0000
+++ include/mir_test_doubles/mock_surface.h	2013-03-12 00:14:21 +0000
@@ -0,0 +1,50 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_TEST_DOUBLES_MOCK_SURFACE_H_
+#define MIR_TEST_DOUBLES_MOCK_SURFACE_H_
+
+#include "mir/shell/surface.h"
+
+#include <memory>
+
+namespace mir
+{
+namespace test
+{
+namespace doubles
+{
+
+struct MockSurface : public shell::Surface
+{
+    MOCK_METHOD0(hide, void());
+    MOCK_METHOD0(show, void());
+    MOCK_METHOD0(destroy, void());
+    MOCK_METHOD0(shutdown, void());
+    MOCK_METHOD0(advance_client_buffer, void());
+
+    MOCK_CONST_METHOD0(size, geometry::Size ());
+    MOCK_CONST_METHOD0(pixel_format, geometry::PixelFormat ());
+    MOCK_CONST_METHOD0(client_buffer, std::shared_ptr<compositor::Buffer> ());
+};
+
+}
+}
+} // namespace mir
+
+#endif // MIR_TEST_DOUBLES_MOCK_SURFACE_H_

=== added file 'include/mir_test_doubles/stub_session.h'
--- include/mir_test_doubles/stub_session.h	1970-01-01 00:00:00 +0000
+++ include/mir_test_doubles/stub_session.h	2013-03-12 00:14:21 +0000
@@ -0,0 +1,63 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_TEST_DOUBLES_STUB_SESSION_H_
+#define MIR_TEST_DOUBLES_STUB_SESSION_H_
+
+#include "mir/shell/session.h"
+
+namespace mir
+{
+namespace test
+{
+namespace doubles
+{
+
+struct StubSession : public shell::Session
+{
+    shell::SurfaceId create_surface(shell::SurfaceCreationParameters const& /* params */)
+    {
+        return shell::SurfaceId{0};
+    }
+    void destroy_surface(shell::SurfaceId /* surface */)
+    {
+    }
+    std::shared_ptr<shell::Surface> get_surface(shell::SurfaceId /* surface */) const
+    {
+        return std::shared_ptr<shell::Surface>();
+    }
+    std::string name()
+    {
+        return std::string();
+    }
+    void shutdown()
+    {
+    }
+    void hide()
+    {
+    }
+    void show()
+    {
+    }
+};
+
+}
+}
+} // namespace mir
+
+#endif // MIR_TEST_DOUBLES_STUB_SESSION_H_

=== added file 'include/mir_test_doubles/stub_session_store.h'
--- include/mir_test_doubles/stub_session_store.h	1970-01-01 00:00:00 +0000
+++ include/mir_test_doubles/stub_session_store.h	2013-03-12 00:14:21 +0000
@@ -0,0 +1,56 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_TEST_DOUBLES_STUB_SESSION_STORE_H_
+#define MIR_TEST_DOUBLES_STUB_SESSION_STORE_H_
+
+#include "mir/shell/session_store.h"
+#include "mir_test_doubles/stub_session.h"
+
+namespace mir
+{
+namespace test
+{
+namespace doubles
+{
+
+class StubSessionStore : public shell::SessionStore
+{
+    std::shared_ptr<shell::Session> open_session(std::string const& /* name */)
+    {
+        return std::make_shared<StubSession>();
+    }
+    void close_session(std::shared_ptr<shell::Session> const& /* session */)
+    {
+    }
+    void tag_session_with_lightdm_id(std::shared_ptr<shell::Session> const& /* session */, int /* id */)
+    {
+    }
+    void focus_session_with_lightdm_id(int /* id */)
+    {
+    }
+    void shutdown()
+    {
+    }
+};
+
+}
+}
+} // namespace mir
+
+#endif // MIR_TEST_DOUBLES_STUB_SESSION_STORE_H_

=== modified file 'tests/integration-tests/cucumber/test_session_management_context.cpp'
--- tests/integration-tests/cucumber/test_session_management_context.cpp	2013-03-07 21:35:30 +0000
+++ tests/integration-tests/cucumber/test_session_management_context.cpp	2013-03-12 00:14:21 +0000
@@ -25,6 +25,7 @@
 #include "mir/graphics/viewable_area.h"
 
 #include "mir_test_doubles/mock_session.h"
+#include "mir_test_doubles/mock_session_store.h"
 #include "mir_test/fake_shared.h"
 
 #include <gtest/gtest.h>
@@ -52,17 +53,6 @@
     MOCK_METHOD0(the_drawer, std::shared_ptr<mc::Drawer>());
 };
 
-struct MockSessionStore : public msh::SessionStore
-{
-    MOCK_METHOD1(open_session, std::shared_ptr<msh::Session>(std::string const&));
-    MOCK_METHOD1(close_session, void(std::shared_ptr<msh::Session> const&));
-
-    MOCK_METHOD2(tag_session_with_lightdm_id, void(std::shared_ptr<msh::Session> const&, int));
-    MOCK_METHOD1(focus_session_with_lightdm_id, void(int));
-
-    MOCK_METHOD0(shutdown, void());
-};
-
 struct MockSurface : public msh::Surface
 {
     MOCK_METHOD0(hide, void());
@@ -109,7 +99,7 @@
         ctx = std::make_shared<mtc::SessionManagementContext>(server_configuration);
     }
     MockServerConfiguration server_configuration;
-    MockSessionStore session_store;
+    mtd::MockSessionStore session_store;
     std::shared_ptr<mtc::SessionManagementContext> ctx;
 
     static msh::SurfaceId const test_surface_id;
@@ -143,7 +133,7 @@
     using namespace ::testing;
 
     MockServerConfiguration server_configuration;
-    MockSessionStore session_store;
+    mtd::MockSessionStore session_store;
     
     EXPECT_CALL(server_configuration, the_session_store()).Times(1)
         .WillOnce(Return(mt::fake_shared<msh::SessionStore>(session_store)));

=== modified file 'tests/unit-tests/frontend/test_application_mediator.cpp'
--- tests/unit-tests/frontend/test_application_mediator.cpp	2013-03-08 19:20:24 +0000
+++ tests/unit-tests/frontend/test_application_mediator.cpp	2013-03-12 00:14:21 +0000
@@ -23,14 +23,16 @@
 #include "mir/frontend/application_mediator.h"
 #include "mir/frontend/resource_cache.h"
 #include "mir/shell/application_session.h"
-#include "mir/shell/session_store.h"
-#include "mir/shell/surface_factory.h"
 #include "mir/graphics/display.h"
 #include "mir/graphics/platform.h"
 #include "mir/graphics/platform_ipc_package.h"
 #include "mir/surfaces/surface.h"
-#include "mir_test_doubles/null_buffer_bundle.h"
 #include "mir_test_doubles/null_display.h"
+#include "mir_test_doubles/mock_session_store.h"
+#include "mir_test_doubles/mock_surface.h"
+#include "mir_test_doubles/stub_buffer.h"
+#include "mir_test_doubles/stub_session.h"
+#include "mir_test/fake_shared.h"
 
 #include "src/surfaces/proxy_surface.h"
 
@@ -46,73 +48,34 @@
 namespace geom = mir::geometry;
 namespace mp = mir::protobuf;
 namespace msh = mir::shell;
-namespace mtd = mir::test::doubles;
+namespace mt = mir::test;
+namespace mtd = mt::doubles;
 
 namespace
 {
 
-/*
- * TODO: Fix design so that it's possible to unit-test ApplicationMediator
- * without having to create doubles for classes so deep in its dependency
- * hierarchy, and needing to resort to ugly tricks to get the information
- * we need (e.g. see DestructionRecordingSession below).
- *
- * In particular, it would be nice if both mf::Session and ms::Surface were
- * stubable/mockable.
- */
-
-class DestructionRecordingSession : public msh::ApplicationSession
-{
-public:
-    DestructionRecordingSession(std::shared_ptr<msh::SurfaceFactory> const& surface_factory)
-        : msh::ApplicationSession{surface_factory, "Stub"}
-    {
-        destroyed = false;
-    }
-
-    ~DestructionRecordingSession() { destroyed = true; }
-
-    static bool destroyed;
-};
-
-bool DestructionRecordingSession::destroyed{true};
-
-class StubSurfaceFactory : public msh::SurfaceFactory
-{
- public:
-    std::shared_ptr<msh::Surface> create_surface(const msh::SurfaceCreationParameters& /*params*/)
-    {
-        auto surface = std::make_shared<ms::Surface>("DummySurface",
-                                                     std::make_shared<mtd::NullBufferBundle>());
-        surfaces.push_back(surface);
-
-        return std::make_shared<ms::BasicProxySurface>(std::weak_ptr<ms::Surface>(surface));
-    }
-
-private:
-    std::vector<std::shared_ptr<ms::Surface>> surfaces;
-};
-
-class StubSessionStore : public msh::SessionStore
-{
-public:
-    StubSessionStore()
-        : factory{std::make_shared<StubSurfaceFactory>()}
-    {
-    }
-
-    std::shared_ptr<msh::Session> open_session(std::string const& /*name*/)
-    {
-        return std::make_shared<DestructionRecordingSession>(factory);
-    }
-
-    void close_session(std::shared_ptr<msh::Session> const& /*session*/) {}
-
-    void shutdown() {}
-    void tag_session_with_lightdm_id(std::shared_ptr<msh::Session> const&, int) {}
-    void focus_session_with_lightdm_id(int) {}
-
-    std::shared_ptr<msh::SurfaceFactory> factory;
+class StubbedSession : public mtd::StubSession
+{
+public:
+    StubbedSession()
+    {
+        using namespace ::testing;
+        
+        mock_surface = std::make_shared<mtd::MockSurface>();
+        
+        EXPECT_CALL(*mock_surface, size()).Times(AnyNumber()).WillRepeatedly(Return(geom::Size()));
+        EXPECT_CALL(*mock_surface, pixel_format()).Times(AnyNumber()).WillRepeatedly(Return(geom::PixelFormat()));
+        EXPECT_CALL(*mock_surface, client_buffer()).Times(AnyNumber()).WillRepeatedly(Return(mt::fake_shared(stub_buffer)));
+        EXPECT_CALL(*mock_surface, advance_client_buffer()).Times(AnyNumber());
+    }
+
+    std::shared_ptr<msh::Surface> get_surface(msh::SurfaceId /* surface */) const
+    {
+        return mock_surface;
+    }
+
+    std::shared_ptr<mtd::MockSurface> mock_surface;
+    mtd::StubBuffer stub_buffer;
 };
 
 class MockGraphicBufferAllocator : public mc::GraphicBufferAllocator
@@ -157,7 +120,7 @@
 struct ApplicationMediatorTest : public ::testing::Test
 {
     ApplicationMediatorTest()
-        : session_store{std::make_shared<StubSessionStore>()},
+        : session_store{std::make_shared<testing::NiceMock<mtd::MockSessionStore>>()},
           graphics_platform{std::make_shared<StubPlatform>()},
           graphics_display{std::make_shared<mtd::NullDisplay>()},
           buffer_allocator{std::make_shared<testing::NiceMock<MockGraphicBufferAllocator>>()},
@@ -167,9 +130,12 @@
                    buffer_allocator, report, resource_cache},
           null_callback{google::protobuf::NewPermanentCallback(google::protobuf::DoNothing)}
     {
+        using namespace ::testing;
+
+        ON_CALL(*session_store, open_session(_)).WillByDefault(Return(std::make_shared<StubbedSession>()));
     }
 
-    std::shared_ptr<msh::SessionStore> const session_store;
+    std::shared_ptr<testing::NiceMock<mtd::MockSessionStore>> const session_store;
     std::shared_ptr<mg::Platform> const graphics_platform;
     std::shared_ptr<mg::Display> const graphics_display;
     std::shared_ptr<testing::NiceMock<MockGraphicBufferAllocator>> const buffer_allocator;
@@ -183,16 +149,15 @@
 
 TEST_F(ApplicationMediatorTest, disconnect_releases_session)
 {
+    using namespace ::testing;
+
     mp::ConnectParameters connect_parameters;
     mp::Connection connection;
+    
+    EXPECT_CALL(*session_store, close_session(_)).Times(1);
 
     mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());
-
-    EXPECT_FALSE(DestructionRecordingSession::destroyed);
-
     mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
-
-    EXPECT_TRUE(DestructionRecordingSession::destroyed);
 }
 
 TEST_F(ApplicationMediatorTest, calling_methods_before_connect_throws)

=== modified file 'tests/unit-tests/frontend/test_application_mediator_android.cpp'
--- tests/unit-tests/frontend/test_application_mediator_android.cpp	2013-03-06 12:54:33 +0000
+++ tests/unit-tests/frontend/test_application_mediator_android.cpp	2013-03-12 00:14:21 +0000
@@ -22,12 +22,14 @@
 #include "mir/frontend/resource_cache.h"
 #include "mir/shell/application_session.h"
 #include "mir/shell/session_store.h"
-#include "mir/shell/surface_factory.h"
+#include "mir/shell/surface_creation_parameters.h"
 #include "mir/graphics/display.h"
 #include "mir/graphics/platform.h"
 #include "mir/graphics/platform_ipc_package.h"
 
 #include "mir_test_doubles/null_display.h"
+#include "mir_test_doubles/mock_session.h"
+#include "mir_test_doubles/stub_session_store.h"
 
 #include <gtest/gtest.h>
 
@@ -44,45 +46,6 @@
 namespace
 {
 
-/*
- * TODO: Fix design so that it's possible to unit-test ApplicationMediator
- * without having to create doubles for classes so deep in its dependency
- * hierarchy.
- *
- * In particular, it would be nice if mf::Session was stubable/mockable.
- */
-
-class StubSurfaceFactory : public msh::SurfaceFactory
-{
- public:
-    std::shared_ptr<msh::Surface> create_surface(const msh::SurfaceCreationParameters& /*params*/)
-    {
-        return std::shared_ptr<msh::Surface>();
-    }
-};
-
-class StubSessionStore : public msh::SessionStore
-{
-public:
-    StubSessionStore()
-        : factory{std::make_shared<StubSurfaceFactory>()}
-    {
-    }
-
-    std::shared_ptr<msh::Session> open_session(std::string const& /*name*/)
-    {
-        return std::make_shared<msh::ApplicationSession>(factory, "stub");
-    }
-
-    void close_session(std::shared_ptr<msh::Session> const& /*session*/) {}
-    void tag_session_with_lightdm_id(std::shared_ptr<msh::Session> const& /*session*/, int /*id*/) {}
-    void focus_session_with_lightdm_id(int /*id*/) {}
-
-    void shutdown() {}
-
-    std::shared_ptr<msh::SurfaceFactory> factory;
-};
-
 class StubGraphicBufferAllocator : public mc::GraphicBufferAllocator
 {
 public:
@@ -122,7 +85,7 @@
 struct ApplicationMediatorAndroidTest : public ::testing::Test
 {
     ApplicationMediatorAndroidTest()
-        : session_store{std::make_shared<StubSessionStore>()},
+        : session_store{std::make_shared<mtd::StubSessionStore>()},
           graphics_platform{std::make_shared<StubPlatform>()},
           graphics_display{std::make_shared<mtd::NullDisplay>()},
           buffer_allocator{std::make_shared<StubGraphicBufferAllocator>()},
@@ -134,7 +97,7 @@
     {
     }
 
-    std::shared_ptr<msh::SessionStore> const session_store;
+    std::shared_ptr<StubSessionStore> const session_store;
     std::shared_ptr<StubPlatform> const graphics_platform;
     std::shared_ptr<mg::Display> const graphics_display;
     std::shared_ptr<mc::GraphicBufferAllocator> const buffer_allocator;

=== modified file 'tests/unit-tests/frontend/test_application_mediator_gbm.cpp'
--- tests/unit-tests/frontend/test_application_mediator_gbm.cpp	2013-03-06 12:54:33 +0000
+++ tests/unit-tests/frontend/test_application_mediator_gbm.cpp	2013-03-12 00:14:21 +0000
@@ -22,7 +22,7 @@
 #include "mir/frontend/resource_cache.h"
 #include "mir/shell/application_session.h"
 #include "mir/shell/session_store.h"
-#include "mir/shell/surface_factory.h"
+#include "mir/shell/surface_creation_parameters.h"
 #include "mir/graphics/display.h"
 #include "mir/graphics/drm_authenticator.h"
 #include "mir/graphics/platform.h"
@@ -32,6 +32,8 @@
 #include <boost/throw_exception.hpp>
 
 #include "mir_test_doubles/null_display.h"
+#include "mir_test_doubles/mock_session.h"
+#include "mir_test_doubles/stub_session_store.h"
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
@@ -47,45 +49,6 @@
 namespace
 {
 
-/*
- * TODO: Fix design so that it's possible to unit-test ApplicationMediator
- * without having to create doubles for classes so deep in its dependency
- * hierarchy.
- *
- * In particular, it would be nice if msh::ApplicationSession was stubable/mockable.
- */
-
-class StubSurfaceFactory : public msh::SurfaceFactory
-{
- public:
-    std::shared_ptr<msh::Surface> create_surface(const msh::SurfaceCreationParameters& /*params*/)
-    {
-        return std::shared_ptr<msh::Surface>();
-    }
-};
-
-class StubSessionStore : public msh::SessionStore
-{
-public:
-    StubSessionStore()
-        : factory{std::make_shared<StubSurfaceFactory>()}
-    {
-    }
-
-    std::shared_ptr<msh::Session> open_session(std::string const& /*name*/)
-    {
-        return std::make_shared<msh::ApplicationSession>(factory, "stub");
-    }
-
-    void close_session(std::shared_ptr<msh::Session> const& /*session*/) {}
-
-    void shutdown() {}
-    void tag_session_with_lightdm_id(std::shared_ptr<msh::Session> const&, int) {}
-    void focus_session_with_lightdm_id(int) {}
-
-    std::shared_ptr<msh::SurfaceFactory> factory;
-};
-
 class StubGraphicBufferAllocator : public mc::GraphicBufferAllocator
 {
 public:
@@ -127,7 +90,7 @@
 struct ApplicationMediatorGBMTest : public ::testing::Test
 {
     ApplicationMediatorGBMTest()
-        : session_store{std::make_shared<StubSessionStore>()},
+        : session_store{std::make_shared<mtd::StubSessionStore>()},
           mock_platform{std::make_shared<MockAuthenticatingPlatform>()},
           graphics_display{std::make_shared<mtd::NullDisplay>()},
           buffer_allocator{std::make_shared<StubGraphicBufferAllocator>()},
@@ -139,7 +102,7 @@
     {
     }
 
-    std::shared_ptr<msh::SessionStore> const session_store;
+    std::shared_ptr<mtd::StubSessionStore> const session_store;
     std::shared_ptr<MockAuthenticatingPlatform> const mock_platform;
     std::shared_ptr<mg::Display> const graphics_display;
     std::shared_ptr<mc::GraphicBufferAllocator> const buffer_allocator;

=== modified file 'tests/unit-tests/shell/test_application_session.cpp'
--- tests/unit-tests/shell/test_application_session.cpp	2013-03-06 12:54:33 +0000
+++ tests/unit-tests/shell/test_application_session.cpp	2013-03-12 00:14:21 +0000
@@ -21,6 +21,7 @@
 #include "mir/shell/surface_creation_parameters.h"
 #include "mir_test/fake_shared.h"
 #include "mir_test_doubles/mock_surface_factory.h"
+#include "mir_test_doubles/mock_surface.h"
 
 #include "src/surfaces/proxy_surface.h"
 
@@ -33,47 +34,11 @@
 namespace mt = mir::test;
 namespace mtd = mir::test::doubles;
 
-namespace
-{
-class MockSurface : public msh::Surface
-{
-public:
-    MockSurface(std::weak_ptr<ms::Surface> const& surface) :
-        impl(surface)
-    {
-        using namespace testing;
-
-        ON_CALL(*this, hide()).WillByDefault(Invoke(&impl, &ms::BasicProxySurface::hide));
-        ON_CALL(*this, show()).WillByDefault(Invoke(&impl, &ms::BasicProxySurface::show));
-        ON_CALL(*this, destroy()).WillByDefault(Invoke(&impl, &ms::BasicProxySurface::destroy));
-        ON_CALL(*this, shutdown()).WillByDefault(Invoke(&impl, &ms::BasicProxySurface::shutdown));
-        ON_CALL(*this, advance_client_buffer()).WillByDefault(Invoke(&impl, &ms::BasicProxySurface::advance_client_buffer));
-
-        ON_CALL(*this, size()).WillByDefault(Invoke(&impl, &ms::BasicProxySurface::size));
-        ON_CALL(*this, pixel_format()).WillByDefault(Invoke(&impl, &ms::BasicProxySurface::pixel_format));
-        ON_CALL(*this, client_buffer()).WillByDefault(Invoke(&impl, &ms::BasicProxySurface::client_buffer));
-    }
-
-    MOCK_METHOD0(hide, void());
-    MOCK_METHOD0(show, void());
-    MOCK_METHOD0(destroy, void());
-    MOCK_METHOD0(shutdown, void());
-    MOCK_METHOD0(advance_client_buffer, void());
-
-    MOCK_CONST_METHOD0(size, mir::geometry::Size ());
-    MOCK_CONST_METHOD0(pixel_format, mir::geometry::PixelFormat ());
-    MOCK_CONST_METHOD0(client_buffer, std::shared_ptr<mc::Buffer> ());
-
-private:
-    ms::BasicProxySurface impl;
-};
-}
-
 TEST(ApplicationSession, create_and_destroy_surface)
 {
     using namespace ::testing;
 
-    auto const mock_surface = std::make_shared<MockSurface>(std::shared_ptr<ms::Surface>());
+    auto const mock_surface = std::make_shared<mtd::MockSurface>();
     mtd::MockSurfaceFactory surface_factory;
     ON_CALL(surface_factory, create_surface(_)).WillByDefault(Return(mock_surface));
 
@@ -93,7 +58,7 @@
 {
     using namespace ::testing;
 
-    auto const mock_surface = std::make_shared<MockSurface>(std::shared_ptr<ms::Surface>());
+    auto const mock_surface = std::make_shared<mtd::MockSurface>();
     mtd::MockSurfaceFactory surface_factory;
     ON_CALL(surface_factory, create_surface(_)).WillByDefault(Return(mock_surface));