Merge lp:~afrantzis/mir/fix-1321215 into lp:mir

[Alexandros Frantzis]

tests: Improve robustness of CustomInputDispatcher tests

Ensure that expectations for our custom input dispatcher are set once when it is first created, in order to avoid racing with the server starting up and using the dispatcher before we have set our expectations.

tests: Improve robustness of CustomInputDispatcher tests

Ensure that expectations for our custom input dispatcher are set once when it is first created, in order to avoid racing with the server starting up and using the dispatcher before we have set our expectations.

[Andreas Pokorny (andreas-pokorny)]

thank you for figuring this out.
looks good.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1643
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1681/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/263
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/264
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/263
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/200
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/200/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/199
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/199/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/681
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/681/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1511
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7391

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1681/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

The relationship between

21 std::shared_ptr<mi::InputDispatcher> the_input_dispatcher() override

and

41 + std::shared_ptr<::testing::NiceMock<CustomMockInputDispatcher>> the_input_dispatcher_mock()

is strange - I'd expect the former to be using the CachePtr not the latter. But that's pre-existing.

OTOH Doing it right would also mean that

57 + bool expectations_set{false};

isn't needed (as the lambda passed to the CachePtr is only executed when the object is created). Vis:

std::shared_ptr<mi::InputDispatcher> the_input_dispatcher() override
{
    return dispatcher(
    [this]
    {
        auto const dispatcher = the_input_dispatcher_mock();

        InSequence seq;
        EXPECT_CALL(*dispatcher, dispatch(mt::MotionEventWithPosition(1, 1))).Times(1);
        EXPECT_CALL(*dispatcher, dispatch(mt::KeyDownEvent()))
            .WillOnce(InvokeWithoutArgs([this]{ dispatching_done.signal_ready(); }));

        return dispatcher;
     });
}

review: Approve

=== modified file 'tests/acceptance-tests/test_custom_input_dispatcher.cpp'
--- tests/acceptance-tests/test_custom_input_dispatcher.cpp	2014-05-20 08:38:05 +0000
+++ tests/acceptance-tests/test_custom_input_dispatcher.cpp	2014-05-20 11:34:30 +0000
@@ -52,12 +52,12 @@
 namespace
 {
 
-class CustomInputDispatcher :
+class CustomMockInputDispatcher :
     public mtd::MockInputDispatcher,
     public msh::InputTargeter
 {
 public:
-    CustomInputDispatcher() = default;
+    CustomMockInputDispatcher() = default;
     // mocks for InputTargeter
     MOCK_METHOD1(focus_changed, void(std::shared_ptr<mi::InputChannel const> const& /*focus_channel*/));
     MOCK_METHOD0(focus_cleared, void());
@@ -67,49 +67,65 @@
 {
     std::shared_ptr<mi::InputDispatcher> the_input_dispatcher() override
     {
-        return the_input_dispatcher_mock();
+        auto const dispatcher = the_input_dispatcher_mock();
+
+        if (!expectations_set)
+        {
+            input_dispatcher_expectations();
+            expectations_set = true;
+        }
+
+        return dispatcher;
     }
+
     std::shared_ptr<msh::InputTargeter> the_input_targeter() override
     {
         return the_input_dispatcher_mock();
     }
-    std::shared_ptr<::testing::NiceMock<CustomInputDispatcher>> the_input_dispatcher_mock()
+
+    std::shared_ptr<::testing::NiceMock<CustomMockInputDispatcher>> the_input_dispatcher_mock()
     {
         return dispatcher(
             [this]
             {
-                return std::make_shared<::testing::NiceMock<CustomInputDispatcher>>();
+                return std::make_shared<::testing::NiceMock<CustomMockInputDispatcher>>();
             });
     }
 
-    mir::CachedPtr<::testing::NiceMock<CustomInputDispatcher>> dispatcher;
+    void inject_input() override {}
+    virtual void input_dispatcher_expectations() {}
+
+    mir::CachedPtr<::testing::NiceMock<CustomMockInputDispatcher>> dispatcher;
     mtf::CrossProcessSync dispatching_done;
+    bool expectations_set{false};
 };
 
 }
 
-using CustomInputDispatcherFixture = BespokeDisplayServerTestFixture;
+using CustomInputDispatcher = BespokeDisplayServerTestFixture;
 
-TEST_F(CustomInputDispatcherFixture, custom_input_dispatcher_receives_input)
+TEST_F(CustomInputDispatcher, receives_input)
 {
     struct ServerConfig : CustomDispatcherServerConfig
     {
         void inject_input() override
         {
-            auto dispatcher = the_input_dispatcher_mock();
-            {
-                using namespace ::testing;
-                InSequence seq;
-                EXPECT_CALL(*dispatcher, dispatch(mt::MotionEventWithPosition(1, 1))).Times(1);
-                EXPECT_CALL(*dispatcher, dispatch(mt::KeyDownEvent()))
-                    .WillOnce(InvokeWithoutArgs([this]{ dispatching_done.signal_ready(); }));
-            }
             fake_event_hub->synthesize_event(mis::a_motion_event().with_movement(1, 1));
             fake_event_hub->synthesize_event(mis::a_key_down_event().of_scancode(KEY_ENTER));
         }
+
+        void input_dispatcher_expectations() override
+        {
+            using namespace ::testing;
+            auto const dispatcher = the_input_dispatcher_mock();
+
+            InSequence seq;
+            EXPECT_CALL(*dispatcher, dispatch(mt::MotionEventWithPosition(1, 1))).Times(1);
+            EXPECT_CALL(*dispatcher, dispatch(mt::KeyDownEvent()))
+                .WillOnce(InvokeWithoutArgs([this]{ dispatching_done.signal_ready(); }));
+        }
     } server_config;
 
-
     launch_server_process(server_config);
 
     // Since event handling happens asynchronously we need to allow some time
@@ -121,35 +137,34 @@
     });
 }
 
-TEST_F(CustomInputDispatcherFixture, custom_input_dispatcher_gets_started_and_stopped)
+TEST_F(CustomInputDispatcher, gets_started_and_stopped)
 {
-    struct ServecConfig : CustomDispatcherServerConfig
+    struct ServerConfig : CustomDispatcherServerConfig
     {
-        void inject_input() override
+        void input_dispatcher_expectations() override
         {
-            auto dispatcher = the_input_dispatcher_mock();
-            {
-                using namespace ::testing;
-                InSequence seq;
-                EXPECT_CALL(*dispatcher, start());
-                EXPECT_CALL(*dispatcher, stop());
-            }
+            using namespace ::testing;
+            auto const dispatcher = the_input_dispatcher_mock();
+
+            InSequence seq;
+            EXPECT_CALL(*dispatcher, start());
+            EXPECT_CALL(*dispatcher, stop());
         }
     } server_config;
 
     launch_server_process(server_config);
 }
 
-TEST_F(CustomInputDispatcherFixture, custom_input_dispatcher_receives_focus_changes)
+TEST_F(CustomInputDispatcher, receives_focus_changes)
 {
-    struct ServecConfig : CustomDispatcherServerConfig
+    struct ServerConfig : CustomDispatcherServerConfig
     {
-        void inject_input() override
+        void input_dispatcher_expectations() override
         {
-            auto dispatcher = the_input_dispatcher_mock();
             using namespace ::testing;
+            auto const dispatcher = the_input_dispatcher_mock();
+
             InSequence seq;
-
             EXPECT_CALL(*dispatcher, focus_cleared()).Times(1);
             EXPECT_CALL(*dispatcher, focus_changed(_)).Times(1);
             EXPECT_CALL(*dispatcher, focus_cleared()).Times(1)