Merge lp:~alan-griffiths/mir/fix-1388802-possibly into lp:mir

[Alan Griffiths]

tests: Make TestClientInput less helgrind unfriendly

tests: Make TestClientInput less helgrind unfriendly

helgrind reports a number of errors around expectations - but many of these seem to be because the thread fulfilling the expectation is running before the expectation is set. There is sequencing (by only sending events from the server after setting expectations) but streaming data over a socket goes beyond the "memory model" and helgrind doesn't see it.

I've changed the tests to start the client thread after the expectations are set - which more than halves the size of the helgrind report.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2039
http://jenkins.qa.ubuntu.com/job/mir-ci/2026/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/15
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/15/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2026/rebuild

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

Sure.

review: Approve

[Chris Halse Rogers (raof)]

I'm not super-enthused about the general principle of making code (slightly) less obviously correct in order to appease an external correctness-checking tool.

In this case it seems a worthwhile tradeoff.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/827/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/25
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/25
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/7
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/7/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3300/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15532

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Suspicious that acceptance tests for this are crashing in CI. That suggests more needs fixing...

pull: /var/crash/_usr_bin_mir_acceptance_tests.32011.uploaded -> ./results/crash/_usr_bin_mir_acceptance_tests.32011.uploaded
failed to copy '/var/crash/_usr_bin_mir_acceptance_tests.32011.uploaded' to './results/crash/_usr_bin_mir_acceptance_tests.32011.uploaded': Permission denied
pull: /var/crash/_usr_bin_mir_acceptance_tests.32011.upload -> ./results/crash/_usr_bin_mir_acceptance_tests.32011.upload
pull: /var/crash/_usr_sbin_unity-system-compositor.0.uploaded -> ./results/crash/_usr_sbin_unity-system-compositor.0.uploaded
failed to copy '/var/crash/_usr_sbin_unity-system-compositor.0.uploaded' to './results/crash/_usr_sbin_unity-system-compositor.0.uploaded': Permission denied
pull: /var/crash/_usr_sbin_unity-system-compositor.0.upload -> ./results/crash/_usr_sbin_unity-system-compositor.0.upload
pull: /var/crash/_usr_bin_mir_acceptance_tests.32011.crash -> ./results/crash/_usr_bin_mir_acceptance_tests.32011.crash
pull: /var/crash/_usr_sbin_unity-system-compositor.0.crash -> ./results/crash/_usr_sbin_unity-system-compositor.0.crash
failed to copy '/var/crash/_usr_sbin_unity-system-compositor.0.crash' to './results/crash/_usr_sbin_unity-system-compositor.0.crash': Permission denied
pull: /var/crash/.lock -> ./results/crash/.lock
7 files pulled. 0 files skipped.

[Alan Griffiths (alan-griffiths)]

The failing test is unrelated to this MP:

[----------] 1 test from StaleFrames
[ RUN ] StaleFrames.are_dropped_when_restarting_compositor
/tmp/buildd/mir-0.8.0+14.10.20141010bzr2039pkg0vivid4+autopilot0/tests/integration-tests/test_stale_frames.cpp:203: Failure
Expected: (stale_buffer_id1) != (stale_buffer_id2), actual: 3 vs 3
/tmp/buildd/mir-0.8.0+14.10.20141010bzr2039pkg0vivid4+autopilot0/tests/integration-tests/test_stale_frames.cpp:204: Failure
Expected: (stale_buffer_id2) != (buffer_id3), actual: 3 vs 3
/tmp/buildd/mir-0.8.0+14.10.20141010bzr2039pkg0vivid4+autopilot0/tests/integration-tests/test_stale_frames.cpp:209: Failure
Value of: new_buffers[0]
Actual: 40
Expected: buffer_id3
Which is: 3
[ FAILED ] StaleFrames.are_dropped_when_restarting_compositor (221 ms)

I guess that needs a bug of its own: lp:1390388

[Alan Griffiths (alan-griffiths)]

(Re-triggering to see what jobs run and what tests fail)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2039
http://jenkins.qa.ubuntu.com/job/mir-ci/2036/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/32
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/32
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/11/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/25
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/25/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/11
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/11/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3306/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15550

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2036/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2039
http://jenkins.qa.ubuntu.com/job/mir-ci/2047/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/44
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/44
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/24
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/36
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/36/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/24
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/24/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3317
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15597

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/2047/rebuild

review: Approve (continuous-integration)

=== modified file 'tests/acceptance-tests/test_client_input.cpp'
--- tests/acceptance-tests/test_client_input.cpp	2014-10-21 16:21:14 +0000
+++ tests/acceptance-tests/test_client_input.cpp	2014-11-06 17:29:14 +0000
@@ -60,18 +60,23 @@
 struct InputClient
 {
     InputClient(std::string const& connect_string, std::string const& client_name)
-        : connect_string{connect_string}, client_name{client_name},
-          client_thread{[this] { run(); }}
+        : connect_string{connect_string}, client_name{client_name}
     {
-        ready_to_accept_events.wait_for_at_most_seconds(5);
     }
 
+
     ~InputClient()
     {
         if (client_thread.joinable())
             client_thread.join();
     }
 
+    void start()
+    {
+        client_thread = std::thread{[this] { run(); }};
+        ready_to_accept_events.wait_for_at_most_seconds(5);
+    }
+
     void run()
     {
         auto const connection =
@@ -231,6 +236,8 @@
 
     int const num_events_produced = 3;
 
+    client.start();
+
     for (int i = 0; i < num_events_produced; i++)
         fake_event_hub()->synthesize_event(
             mis::a_key_down_event().of_scancode(KEY_ENTER));
@@ -252,6 +259,8 @@
                     AllOf(mt::KeyDownEvent(), mt::KeyOfSymbol(XKB_KEY_dollar))))
         .WillOnce(mt::WakeUp(&client.all_events_received));
 
+    client.start();
+
     fake_event_hub()->synthesize_event(
         mis::a_key_down_event().of_scancode(KEY_LEFTSHIFT));
     fake_event_hub()->synthesize_event(
@@ -276,6 +285,8 @@
         .WillOnce(mt::WakeUp(&client.all_events_received));
     // But we should not receive an event for the second movement outside of our surface!
 
+    client.start();
+
     fake_event_hub()->synthesize_event(
         mis::a_motion_event().with_movement(
             InputClient::surface_width - 1,
@@ -293,6 +304,8 @@
     EXPECT_CALL(client.handler, handle_input(mt::ButtonDownEvent(0, 0)))
         .WillOnce(mt::WakeUp(&client.all_events_received));
 
+    client.start();
+
     fake_event_hub()->synthesize_event(
         mis::a_button_down_event()
             .of_button(BTN_LEFT)
@@ -335,6 +348,9 @@
             .WillOnce(mt::WakeUp(&client2.all_events_received));
     }
 
+    client1.start();
+    client2.start();
+
     // In the bounds of the first surface
     fake_event_hub()->synthesize_event(
         mis::a_motion_event().with_movement(screen_width / 2 - 1, screen_height / 2 - 1));
@@ -370,6 +386,8 @@
             .WillOnce(mt::WakeUp(&client.all_events_received));
     }
 
+    client.start();
+
     // First we will move the cursor in to the input region on the left side of
     // the window. We should see a click here.
     fake_event_hub()->synthesize_event(mis::a_motion_event().with_movement(1, 1));
@@ -434,6 +452,9 @@
             .WillOnce(mt::WakeUp(&client2.all_events_received));
     }
 
+    client1.start();
+    client2.start();
+
     // First we will move the cursor in to the region where client 2 obscures client 1
     fake_event_hub()->synthesize_event(mis::a_motion_event().with_movement(1, 1));
     fake_event_hub()->synthesize_event(
@@ -469,6 +490,9 @@
         .WillOnce(DoAll(mt::WakeUp(&second_client_done),
                         mt::WakeUp(&client2.all_events_received)));
 
+    client1.start();
+    client2.start();
+
     // We send one event and then hide the surface on top before sending the next.
     // So we expect each of the two surfaces to receive one even
     fake_event_hub()->synthesize_event(mis::a_motion_event().with_movement(1,1));
@@ -506,6 +530,8 @@
         .Times(AnyNumber())
         .WillOnce(mt::WakeUp(&client1.all_events_received));
 
+    client1.start();
+
     server_config().the_session_container()->for_each(
         [&](std::shared_ptr<ms::Session> const& session) -> void
         {
@@ -559,6 +585,8 @@
                                    expected_motion_y_2)))
         .WillOnce(mt::WakeUp(&client1.all_events_received));
 
+    client1.start();
+
     fake_event_hub()->synthesize_event(
         mis::a_touch_event().at_position({abs_touch_x_1, abs_touch_y_1}));
     // Sleep here to trigger more failures (simulate slow machine)
@@ -574,6 +602,8 @@
     EXPECT_CALL(client1.handler, handle_input(mt::KeyDownEvent()))
         .WillOnce(mt::WakeUp(&client1.all_events_received));
 
+    client1.start();
+
     server_config().the_session_container()->for_each(
         [] (std::shared_ptr<ms::Session> const& session) -> void
         {