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

[Alan Griffiths]

tests: remove a test that was not worth maintaining

tests: remove a test that was not worth maintaining

This test was written to prove we could send FDs over a socket. Even without this test we'd soon notice if that wasn't working!

1. much of the code tested is actually in the test, not production code.
2. this code was failing intermittently lp:1300653 (although I've never been able to reproduce)
3. the code is murky and self-documented as being "a mess"

It really isn't worth the time to fix it.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1744
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2075/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/823
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/829
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/822
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/596
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/596/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/594
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/594/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2586
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/2586/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1971
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/9318

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Yep, looks like a redundant test in the least. And removing it will certainly solve LP: #1300653.

review: Approve

=== modified file 'src/server/frontend/protobuf_message_processor.cpp'
--- src/server/frontend/protobuf_message_processor.cpp	2014-06-11 16:11:32 +0000
+++ src/server/frontend/protobuf_message_processor.cpp	2014-07-04 16:54:18 +0000
@@ -151,10 +151,6 @@
         {
             invoke(this, display_server.get(), &DisplayServer::release_surface, invocation);
         }
-        else if ("test_file_descriptors" == invocation.method_name())
-        {
-            invoke(this, display_server.get(), &DisplayServer::test_file_descriptors, invocation);
-        }
         else if ("drm_auth_magic" == invocation.method_name())
         {
             invoke(this, display_server.get(), &DisplayServer::drm_auth_magic, invocation);

=== modified file 'src/shared/protobuf/mir_protobuf.proto'
--- src/shared/protobuf/mir_protobuf.proto	2014-06-12 10:06:11 +0000
+++ src/shared/protobuf/mir_protobuf.proto	2014-07-04 16:54:18 +0000
@@ -211,8 +211,6 @@
   // Platform specific requests
   rpc drm_auth_magic(DRMMagic) returns (DRMAuthMagicStatus);
 
-  rpc test_file_descriptors(Void) returns (Buffer);
-
   rpc configure_surface(SurfaceSetting) returns (SurfaceSetting);
   rpc configure_display(DisplayConfiguration) returns (DisplayConfiguration);
 

=== modified file 'tests/integration-tests/test_error_reporting.cpp'
--- tests/integration-tests/test_error_reporting.cpp	2014-06-03 11:04:15 +0000
+++ tests/integration-tests/test_error_reporting.cpp	2014-07-04 16:54:18 +0000
@@ -88,15 +88,6 @@
     {
         throw std::runtime_error(test_exception_text);
     }
-
-    void test_file_descriptors(
-        google::protobuf::RpcController*,
-        const mir::protobuf::Void*,
-        mir::protobuf::Buffer*,
-        google::protobuf::Closure*)
-    {
-        throw std::runtime_error(test_exception_text);
-    }
 };
 
 std::string const ErrorServer::test_exception_text{"test exception text"};

=== modified file 'tests/unit-tests/frontend/CMakeLists.txt'
--- tests/unit-tests/frontend/CMakeLists.txt	2014-06-04 15:31:35 +0000
+++ tests/unit-tests/frontend/CMakeLists.txt	2014-07-04 16:54:18 +0000
@@ -13,18 +13,6 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_basic_connector.cpp
 )
 
-# TODO this test is a mess - something better is needed.
-# - it doesn't pass FDs between processes
-# - it doesn't test that the received FD can be used
-# - the code it tests [test_file_descriptors()] isn't part of the system proper
-# - it touches the filesystem
-# Really, it tests for a side-effect of passing an FD over a socket using libancillary.
-# So, we should write an integration level test that passes FDs between
-# processes and get rid of this test and test_file_descriptors()
-list(APPEND UNIT_TEST_SOURCES
-  ${CMAKE_CURRENT_SOURCE_DIR}/test_protobuf_sends_fds.cpp
-)
-
 if (MIR_TEST_PLATFORM STREQUAL "android")
   list(APPEND UNIT_TEST_SOURCES
     ${CMAKE_CURRENT_SOURCE_DIR}/test_session_mediator_android.cpp

=== modified file 'tests/unit-tests/frontend/test_protobuf_reports_errors.cpp'
--- tests/unit-tests/frontend/test_protobuf_reports_errors.cpp	2014-03-06 06:05:17 +0000
+++ tests/unit-tests/frontend/test_protobuf_reports_errors.cpp	2014-07-04 16:54:18 +0000
@@ -72,15 +72,6 @@
     {
         throw std::runtime_error(test_exception_text);
     }
-
-    void test_file_descriptors(
-        google::protobuf::RpcController*,
-        const protobuf::Void*,
-        protobuf::Buffer*,
-        google::protobuf::Closure*)
-    {
-        throw std::runtime_error(test_exception_text);
-    }
 };
 
 std::string const ErrorServer::test_exception_text{"test exception text"};

=== removed file 'tests/unit-tests/frontend/test_protobuf_sends_fds.cpp'
--- tests/unit-tests/frontend/test_protobuf_sends_fds.cpp	2014-03-06 06:05:17 +0000
+++ tests/unit-tests/frontend/test_protobuf_sends_fds.cpp	1970-01-01 00:00:00 +0000
@@ -1,137 +0,0 @@
-/*
- * Copyright © 2012 Canonical Ltd.
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 3 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- *
- * Authored by: Thomas Voss <[e-mail]>
- *              Alan Griffiths <[e-mail]>
- */
-
-#include "mir/frontend/connector.h"
-#include "src/server/frontend/resource_cache.h"
-
-#include "mir_protobuf.pb.h"
-
-#include "mir_test_doubles/stub_ipc_factory.h"
-#include "mir_test/stub_server_tool.h"
-#include "mir_test/test_protobuf_client.h"
-#include "mir_test/test_protobuf_server.h"
-
-#include <gtest/gtest.h>
-#include <gmock/gmock.h>
-
-#include <fcntl.h>
-
-#include <stdexcept>
-#include <memory>
-#include <string>
-
-namespace mf = mir::frontend;
-namespace mt = mir::test;
-
-namespace mir
-{
-namespace test
-{
-struct StubServerFd : public StubServerTool
-{
-    static const int file_descriptors = 5;
-    int file_descriptor[file_descriptors];
-
-    StubServerFd()
-    {
-        for (auto i = file_descriptor; i != file_descriptor+file_descriptors; ++i)
-            *i = 0;
-    }
-
-    void test_file_descriptors(::google::protobuf::RpcController* ,
-                         const ::mir::protobuf::Void* ,
-                         ::mir::protobuf::Buffer* buffer,
-                         ::google::protobuf::Closure* done)
-    {
-        for (int i = 0; i != file_descriptors; ++i)
-        {
-
-            static char const test_file_fmt[] = "fd_test_file%d";
-            char test_file[sizeof test_file_fmt];
-            sprintf(test_file, test_file_fmt, i);
-            remove(test_file);
-            file_descriptor[i] = open(test_file, O_CREAT, S_IWUSR|S_IRUSR);
-
-            buffer->add_fd(file_descriptor[i]);
-        }
-
-        done->Run();
-    }
-
-    void close_files()
-    {
-        for (auto i = file_descriptor; i != file_descriptor+file_descriptors; ++i)
-            close(*i), *i = 0;
-    }
-
-};
-const int StubServerFd::file_descriptors;
-}
-
-struct ProtobufSocketCommunicatorFD : public ::testing::Test
-{
-    void SetUp()
-    {
-        stub_server_tool = std::make_shared<mt::StubServerFd>();
-        stub_server = std::make_shared<mt::TestProtobufServer>("./test_socket", stub_server_tool);
-        stub_server->comm->start();
-
-        stub_client = std::make_shared<mt::TestProtobufClient>("./test_socket", 500);
-        stub_client->connect_parameters.set_application_name(__PRETTY_FUNCTION__);
-    }
-
-    void TearDown()
-    {
-        stub_server.reset();
-    }
-
-    std::shared_ptr<mt::TestProtobufClient> stub_client;
-    std::shared_ptr<mt::StubServerFd> stub_server_tool;
-
-private:
-    std::shared_ptr<mt::TestProtobufServer> stub_server;
-};
-
-TEST_F(ProtobufSocketCommunicatorFD, test_file_descriptors)
-{
-    mir::protobuf::Buffer fds;
-
-    stub_client->display_server.test_file_descriptors(0, &stub_client->ignored, &fds,
-        google::protobuf::NewCallback(stub_client.get(), &mt::TestProtobufClient::tfd_done));
-
-    stub_client->wait_for_tfd_done();
-
-    ASSERT_EQ(stub_server_tool->file_descriptors, fds.fd_size());
-
-    for (int i  = 0; i != stub_server_tool->file_descriptors; ++i)
-    {
-        int const fd = fds.fd(i);
-        EXPECT_NE(-1, fd);
-
-        for (int j  = 0; j != stub_server_tool->file_descriptors; ++j)
-        {
-            EXPECT_NE(stub_server_tool->file_descriptor[j], fd);
-        }
-    }
-
-    stub_server_tool->close_files();
-    for (int i  = 0; i != stub_server_tool->file_descriptors; ++i)
-        close(fds.fd(i));
-}
-}