Merge lp:~alan-griffiths/mir/fix-1245336 into lp:mir

[Alan Griffiths]

Avoid a race condition that could lead to spurious failures of server
shutdown tests (LP: #1245336)

The race is between the acceptance-tests process and its child server process.
In our signal/abort tests, it's possible the server process may start and
stop so quickly that the parent process doesn't have time to detect the
socket file (before it's correctly cleaned up).

tests: Use CrossProcessSync to avoid race condition

Ensure that the test process has detected the endpoint on the filesystem before terminating the server

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1176
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/286/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/43
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/42
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/15
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/15/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/15
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/15/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/286/rebuild

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

+1

review: Approve

[Daniel van Vugt (vanvugt)]

Nice, thanks.

Simplicity is good. Though I notice the implementation of CrossProcessSync is actually much more complex than my proposed implementation. It looks like it could be more powerful too.

review: Approve

=== modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
--- tests/acceptance-tests/test_server_shutdown.cpp	2013-10-25 09:01:53 +0000
+++ tests/acceptance-tests/test_server_shutdown.cpp	2013-10-29 16:38:44 +0000
@@ -23,6 +23,7 @@
 
 #include "mir_test_framework/display_server_test_fixture.h"
 #include "mir_test/fake_event_hub_input_configuration.h"
+#include "mir_test_framework/cross_process_sync.h"
 
 #include <gtest/gtest.h>
 
@@ -375,8 +376,11 @@
     {
         void exec() override
         {
+            sync.wait_for_signal_ready_for();
             abort();
         }
+
+        mtf::CrossProcessSync sync;
     };
 
     ServerConfig server_config;
@@ -384,11 +388,10 @@
 
     run_in_test_process([&]
     {
-        using std::chrono::steady_clock;
-        std::chrono::seconds const limit(2);
-
         ASSERT_TRUE(file_exists(server_config.the_socket_file()));
 
+        server_config.sync.signal_ready();
+
         // shutdown should fail as the server aborted
         ASSERT_FALSE(shutdown_server_process());
 
@@ -404,11 +407,14 @@
     {
         void exec() override
         {
+            sync.wait_for_signal_ready_for();
             raise(sig);
         }
 
         ServerConfig(int sig) : sig(sig) {}
+
         int const sig;
+        mtf::CrossProcessSync sync;
     };
 
     ServerConfig server_config(GetParam());
@@ -416,11 +422,10 @@
 
     run_in_test_process([&]
     {
-        using std::chrono::steady_clock;
-        std::chrono::seconds const limit(2);
-
         ASSERT_TRUE(file_exists(server_config.the_socket_file()));
 
+        server_config.sync.signal_ready();
+
         // shutdown should fail as the server faulted
         ASSERT_FALSE(shutdown_server_process());