Mir

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

  1. fix-1388802-possibly
  2. Merge into development-branch
Proposed by Alan Griffiths
Status: Merged
Approved by: Alan Griffiths
Approved revision: no longer in the source branch.
Merged at revision: 2040
Proposed branch: lp:~alan-griffiths/mir/fix-1388802-possibly
Merge into: lp:mir
Diff against target: 132 lines (+33/-3)
1 file modified
tests/acceptance-tests/test_client_input.cpp (+33/-3)
To merge this branch: bzr merge lp:~alan-griffiths/mir/fix-1388802-possibly
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Chris Halse Rogers Approve
Alberto Aguirre (community) Approve
Review via email: mp+240926@code.launchpad.net

Commit message

tests: Make TestClientInput less helgrind unfriendly

Description of the change

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.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

Sure.

review: Approve
Revision history for this message
Chris Halse Rogers (raof) wrote :

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
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

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

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

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

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
60struct InputClient60struct InputClient
61{61{
62 InputClient(std::string const& connect_string, std::string const& client_name)62 InputClient(std::string const& connect_string, std::string const& client_name)
63 : connect_string{connect_string}, client_name{client_name},63 : connect_string{connect_string}, client_name{client_name}
64 client_thread{[this] { run(); }}
65 {64 {
66 ready_to_accept_events.wait_for_at_most_seconds(5);
67 }65 }
6866
67
69 ~InputClient()68 ~InputClient()
70 {69 {
71 if (client_thread.joinable())70 if (client_thread.joinable())
72 client_thread.join();71 client_thread.join();
73 }72 }
7473
74 void start()
75 {
76 client_thread = std::thread{[this] { run(); }};
77 ready_to_accept_events.wait_for_at_most_seconds(5);
78 }
79
75 void run()80 void run()
76 {81 {
77 auto const connection =82 auto const connection =
@@ -231,6 +236,8 @@
231236
232 int const num_events_produced = 3;237 int const num_events_produced = 3;
233238
239 client.start();
240
234 for (int i = 0; i < num_events_produced; i++)241 for (int i = 0; i < num_events_produced; i++)
235 fake_event_hub()->synthesize_event(242 fake_event_hub()->synthesize_event(
236 mis::a_key_down_event().of_scancode(KEY_ENTER));243 mis::a_key_down_event().of_scancode(KEY_ENTER));
@@ -252,6 +259,8 @@
252 AllOf(mt::KeyDownEvent(), mt::KeyOfSymbol(XKB_KEY_dollar))))259 AllOf(mt::KeyDownEvent(), mt::KeyOfSymbol(XKB_KEY_dollar))))
253 .WillOnce(mt::WakeUp(&client.all_events_received));260 .WillOnce(mt::WakeUp(&client.all_events_received));
254261
262 client.start();
263
255 fake_event_hub()->synthesize_event(264 fake_event_hub()->synthesize_event(
256 mis::a_key_down_event().of_scancode(KEY_LEFTSHIFT));265 mis::a_key_down_event().of_scancode(KEY_LEFTSHIFT));
257 fake_event_hub()->synthesize_event(266 fake_event_hub()->synthesize_event(
@@ -276,6 +285,8 @@
276 .WillOnce(mt::WakeUp(&client.all_events_received));285 .WillOnce(mt::WakeUp(&client.all_events_received));
277 // But we should not receive an event for the second movement outside of our surface!286 // But we should not receive an event for the second movement outside of our surface!
278287
288 client.start();
289
279 fake_event_hub()->synthesize_event(290 fake_event_hub()->synthesize_event(
280 mis::a_motion_event().with_movement(291 mis::a_motion_event().with_movement(
281 InputClient::surface_width - 1,292 InputClient::surface_width - 1,
@@ -293,6 +304,8 @@
293 EXPECT_CALL(client.handler, handle_input(mt::ButtonDownEvent(0, 0)))304 EXPECT_CALL(client.handler, handle_input(mt::ButtonDownEvent(0, 0)))
294 .WillOnce(mt::WakeUp(&client.all_events_received));305 .WillOnce(mt::WakeUp(&client.all_events_received));
295306
307 client.start();
308
296 fake_event_hub()->synthesize_event(309 fake_event_hub()->synthesize_event(
297 mis::a_button_down_event()310 mis::a_button_down_event()
298 .of_button(BTN_LEFT)311 .of_button(BTN_LEFT)
@@ -335,6 +348,9 @@
335 .WillOnce(mt::WakeUp(&client2.all_events_received));348 .WillOnce(mt::WakeUp(&client2.all_events_received));
336 }349 }
337350
351 client1.start();
352 client2.start();
353
338 // In the bounds of the first surface354 // In the bounds of the first surface
339 fake_event_hub()->synthesize_event(355 fake_event_hub()->synthesize_event(
340 mis::a_motion_event().with_movement(screen_width / 2 - 1, screen_height / 2 - 1));356 mis::a_motion_event().with_movement(screen_width / 2 - 1, screen_height / 2 - 1));
@@ -370,6 +386,8 @@
370 .WillOnce(mt::WakeUp(&client.all_events_received));386 .WillOnce(mt::WakeUp(&client.all_events_received));
371 }387 }
372388
389 client.start();
390
373 // First we will move the cursor in to the input region on the left side of391 // First we will move the cursor in to the input region on the left side of
374 // the window. We should see a click here.392 // the window. We should see a click here.
375 fake_event_hub()->synthesize_event(mis::a_motion_event().with_movement(1, 1));393 fake_event_hub()->synthesize_event(mis::a_motion_event().with_movement(1, 1));
@@ -434,6 +452,9 @@
434 .WillOnce(mt::WakeUp(&client2.all_events_received));452 .WillOnce(mt::WakeUp(&client2.all_events_received));
435 }453 }
436454
455 client1.start();
456 client2.start();
457
437 // First we will move the cursor in to the region where client 2 obscures client 1458 // First we will move the cursor in to the region where client 2 obscures client 1
438 fake_event_hub()->synthesize_event(mis::a_motion_event().with_movement(1, 1));459 fake_event_hub()->synthesize_event(mis::a_motion_event().with_movement(1, 1));
439 fake_event_hub()->synthesize_event(460 fake_event_hub()->synthesize_event(
@@ -469,6 +490,9 @@
469 .WillOnce(DoAll(mt::WakeUp(&second_client_done),490 .WillOnce(DoAll(mt::WakeUp(&second_client_done),
470 mt::WakeUp(&client2.all_events_received)));491 mt::WakeUp(&client2.all_events_received)));
471492
493 client1.start();
494 client2.start();
495
472 // We send one event and then hide the surface on top before sending the next.496 // We send one event and then hide the surface on top before sending the next.
473 // So we expect each of the two surfaces to receive one even497 // So we expect each of the two surfaces to receive one even
474 fake_event_hub()->synthesize_event(mis::a_motion_event().with_movement(1,1));498 fake_event_hub()->synthesize_event(mis::a_motion_event().with_movement(1,1));
@@ -506,6 +530,8 @@
506 .Times(AnyNumber())530 .Times(AnyNumber())
507 .WillOnce(mt::WakeUp(&client1.all_events_received));531 .WillOnce(mt::WakeUp(&client1.all_events_received));
508532
533 client1.start();
534
509 server_config().the_session_container()->for_each(535 server_config().the_session_container()->for_each(
510 [&](std::shared_ptr<ms::Session> const& session) -> void536 [&](std::shared_ptr<ms::Session> const& session) -> void
511 {537 {
@@ -559,6 +585,8 @@
559 expected_motion_y_2)))585 expected_motion_y_2)))
560 .WillOnce(mt::WakeUp(&client1.all_events_received));586 .WillOnce(mt::WakeUp(&client1.all_events_received));
561587
588 client1.start();
589
562 fake_event_hub()->synthesize_event(590 fake_event_hub()->synthesize_event(
563 mis::a_touch_event().at_position({abs_touch_x_1, abs_touch_y_1}));591 mis::a_touch_event().at_position({abs_touch_x_1, abs_touch_y_1}));
564 // Sleep here to trigger more failures (simulate slow machine)592 // Sleep here to trigger more failures (simulate slow machine)
@@ -574,6 +602,8 @@
574 EXPECT_CALL(client1.handler, handle_input(mt::KeyDownEvent()))602 EXPECT_CALL(client1.handler, handle_input(mt::KeyDownEvent()))
575 .WillOnce(mt::WakeUp(&client1.all_events_received));603 .WillOnce(mt::WakeUp(&client1.all_events_received));
576604
605 client1.start();
606
577 server_config().the_session_container()->for_each(607 server_config().the_session_container()->for_each(
578 [] (std::shared_ptr<ms::Session> const& session) -> void608 [] (std::shared_ptr<ms::Session> const& session) -> void
579 {609 {

Subscribers

People subscribed via source and target branches