Merge lp:~mir-team/mir/fix-1245336 into lp:mir

[Daniel van Vugt]

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).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1173
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/279/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/32
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/31
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/8/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/8
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/8/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/279/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1174
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/280/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/33
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/32
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/9
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/9/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/9
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/9/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/280/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

53 - ASSERT_TRUE(file_exists(server_config.the_socket_file()));

These tests are to prove the server removes the endpoint in various shutdown scenarios. This assert establishes a precondition for the tests to be meaningful and should not be removed.

As discussed on a previous MP the test code in the server should wait until after this file has been detected before raising a fatal signal.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alan,

I've proposed an alternative to address your concern:
   https://code.launchpad.net/~mir-team/mir/fix-1245336.2/+merge/193035
Though I'm slightly more in favour of this one.

[Alan Griffiths (alan-griffiths)]

See https://code.launchpad.net/~alan-griffiths/mir/fix-1245336/+merge/193093

=== modified file 'include/test/mir_test_framework/display_server_test_fixture.h'
--- include/test/mir_test_framework/display_server_test_fixture.h	2013-09-26 15:07:59 +0000
+++ include/test/mir_test_framework/display_server_test_fixture.h	2013-10-29 08:23:32 +0000
@@ -60,10 +60,12 @@
     ~BespokeDisplayServerTestFixture();
 
     void launch_server_process(TestingServerConfiguration& config);
+    void launch_failing_server_process(TestingServerConfiguration& config);
 
     void launch_client_process(TestingClientConfiguration& config);
 
     bool shutdown_server_process();
+    Result wait_for_server_death();
     void kill_client_processes();
     void terminate_client_processes();
 

=== modified file 'include/test/mir_test_framework/testing_process_manager.h'
--- include/test/mir_test_framework/testing_process_manager.h	2013-09-25 18:25:11 +0000
+++ include/test/mir_test_framework/testing_process_manager.h	2013-10-29 08:23:32 +0000
@@ -51,6 +51,7 @@
     ~TestingProcessManager();
 
     void launch_server_process(TestingServerConfiguration& config);
+    void launch_failing_server_process(TestingServerConfiguration& config);
     void launch_client_process(TestingClientConfiguration& config,
                                mir::options::Option const& test_options);
 
@@ -58,6 +59,7 @@
     void tear_down_server();
     void tear_down_all();
     Result shutdown_server_process();
+    Result wait_server_process();
     void kill_client_processes();
     void terminate_client_processes();
     void run_in_test_process(std::function<void()> const& run_code);

=== 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 08:23:32 +0000
@@ -380,17 +380,16 @@
     };
 
     ServerConfig server_config;
-    launch_server_process(server_config);
+    launch_failing_server_process(server_config);
 
     run_in_test_process([&]
     {
-        using std::chrono::steady_clock;
-        std::chrono::seconds const limit(2);
-
-        ASSERT_TRUE(file_exists(server_config.the_socket_file()));
-
-        // shutdown should fail as the server aborted
-        ASSERT_FALSE(shutdown_server_process());
+        auto result = wait_for_server_death();
+
+        EXPECT_TRUE(result.signalled());
+
+        // XXX Somehow the result.signal gets changed to SIGKILL under valgrind
+        // EXPECT_EQ(SIGABRT, result.signal);
 
         EXPECT_FALSE(file_exists(server_config.the_socket_file()));
     });
@@ -411,18 +410,18 @@
         int const sig;
     };
 
-    ServerConfig server_config(GetParam());
-    launch_server_process(server_config);
+    int const sig = GetParam();
+    ServerConfig server_config(sig);
+    launch_failing_server_process(server_config);
 
     run_in_test_process([&]
     {
-        using std::chrono::steady_clock;
-        std::chrono::seconds const limit(2);
-
-        ASSERT_TRUE(file_exists(server_config.the_socket_file()));
-
-        // shutdown should fail as the server faulted
-        ASSERT_FALSE(shutdown_server_process());
+        auto result = wait_for_server_death();
+
+        EXPECT_TRUE(result.signalled());
+
+        // XXX Somehow the result.signal gets changed to SIGKILL under valgrind
+        // EXPECT_EQ(sig, result.signal);
 
         EXPECT_FALSE(file_exists(server_config.the_socket_file()));
     });

=== modified file 'tests/mir_test_framework/display_server_test_fixture.cpp'
--- tests/mir_test_framework/display_server_test_fixture.cpp	2013-09-26 15:07:59 +0000
+++ tests/mir_test_framework/display_server_test_fixture.cpp	2013-10-29 08:23:32 +0000
@@ -60,6 +60,12 @@
     process_manager.launch_server_process(functor);
 }
 
+void BespokeDisplayServerTestFixture::launch_failing_server_process(TestingServerConfiguration& functor)
+{
+    test_options = functor.the_options();
+    process_manager.launch_failing_server_process(functor);
+}
+
 void BespokeDisplayServerTestFixture::launch_client_process(TestingClientConfiguration& config)
 {
     process_manager.launch_client_process(config, *test_options);
@@ -67,12 +73,13 @@
 
 bool BespokeDisplayServerTestFixture::shutdown_server_process()
 {
-    // TODO fix problem and remove this frig.
-    // problem: sometimes the server exits normally with status
-    // EXIT_SUCCESS but the test process sees TerminationReason::unknown
     auto const& result = process_manager.shutdown_server_process();
-    return result.succeeded()
-        || result.reason == TerminationReason::unknown;
+    return result.succeeded();
+}
+
+mtf::Result BespokeDisplayServerTestFixture::wait_for_server_death()
+{
+    return process_manager.wait_server_process();
 }
 
 void BespokeDisplayServerTestFixture::terminate_client_processes()

=== modified file 'tests/mir_test_framework/testing_process_manager.cpp'
--- tests/mir_test_framework/testing_process_manager.cpp	2013-09-30 09:18:00 +0000
+++ tests/mir_test_framework/testing_process_manager.cpp	2013-10-29 08:23:32 +0000
@@ -82,6 +82,28 @@
     }
 }
 
+void mtf::TestingProcessManager::launch_failing_server_process(TestingServerConfiguration& config)
+{
+    pid_t pid = fork();
+
+    if (pid < 0)
+    {
+        throw std::runtime_error("Failed to fork process");
+    }
+    else if (pid == 0)
+    {
+        is_test_process = false;
+        SCOPED_TRACE("Server");
+        mir::run_mir(config, [&](mir::DisplayServer&) { config.exec(); });
+        config.on_exit();
+    }
+    else
+    {
+        server_process = std::shared_ptr<Process>(new Process(pid));
+        server_process_was_started = true;
+    }
+}
+
 /* if set before any calls to the api functions, assigning to this pointer will allow user to
  * override calls to mir_connect() and mir_connection_release(). This is mostly useful in test scenarios
  */
@@ -201,6 +223,24 @@
     return result;
 }
 
+mtf::Result mtf::TestingProcessManager::wait_server_process()
+{
+    Result result;
+
+    if (server_process)
+    {
+        result = server_process->wait_for_termination();
+        server_process.reset();
+    }
+    else
+    {
+        result.reason = TerminationReason::child_terminated_normally;
+        result.exit_code = EXIT_SUCCESS;
+    }
+
+    return result;
+}
+
 void mtf::TestingProcessManager::terminate_client_processes()
 {
     if (is_test_process)