Merge lp:~kdub/mir/fix-1220443 into lp:~mir-team/mir/trunk

[Kevin DuBois]

fix: lp:1220443

let the test framework to inject stub graphics platforms on the client side during acceptance and integration tests

fix: lp:1220443

let the test framework to inject stub graphics platforms on the client side during acceptance and integration tests

see bug comment for what was happening: https://bugs.launchpad.net/mir/+bug/1220443/comments/3

nb. fix addresses 1220443, but tests-use-real-graphics is still problematic when turned on (https://bugs.launchpad.net/mir/+bug/1221373)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1056
http://jenkins.qa.ubuntu.com/job/mir-ci/1442/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1947
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1832
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/680/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1057
http://jenkins.qa.ubuntu.com/job/mir-ci/1443/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1948
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1833
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/681/console

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

mir_client_library_debug.h: Incorrect capitalization and punctuation.

mir_connection_factory.h: Incorrect date.

They're not blockers, but should be fixed. I haven't stopped long enough to consider the overall approach however, so abstain for now.

review: Abstain

[Alan Griffiths (alan-griffiths)]

Defining MirConnectionFactory in a private header makes publishing mir_global_connection_factory via the debug API pointless - it is unusable.

Initializing mir_global_connection_factory to point to a default would avoid tests for NULL (should be nullptr).

But a better approach would be to publish a function pointer with the same signature as mir_connect() and have mir_connect() forward to whatever it points at. This pointer should default to a renamed copy of the current implementation.

This would allow clients to intercept the call and optionally call the default implementation (or, in test cases, restore the pointer to use).

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1058
http://jenkins.qa.ubuntu.com/job/mir-ci/1457/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1966
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1851
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/695
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/695/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1060
http://jenkins.qa.ubuntu.com/job/mir-ci/1458/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1967/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1852/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/696/console

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

I'm not sure that exposing the client API as something a client can *modify* is a good idea...

11 +extern MirWaitHandle* (*mir_connect_impl)(
12 + char const *server,
13 + char const *app_name,
14 + mir_connected_callback callback,
15 + void *context);

If nothing else, it's inconsistent to only allow this on one function. Normally you would use LD_PRELOAD (man ld.so) to achieve this.

Also, still... mir_client_library_debug.h: Incorrect capitalization and punctuation.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1061
http://jenkins.qa.ubuntu.com/job/mir-ci/1460/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1970
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1855
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/698
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/698/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> I'm not sure that exposing the client API as something a client can *modify*
> is a good idea...

The key question is: is this needed as part of the debug API, or is it a facility for test code that should be internal to the implementation?

If the former, then we should probably providing a mechanism for intercepting the whole API atomically, not a single functions.

*Needs discussion*

review: Needs Information

[Kevin DuBois (kdub)]

I think its more of a test code facility, so perhaps it should be moved out of the debug header.
Since running with stubbed drivers is the default, I'm not a fan of recommending the use of LD_PRELOAD to anyone who wants to run the android test suite.

[Daniel van Vugt (vanvugt)]

You should be able to do the equivalent of LD_PRELOAD without using the environment variable, maybe? The linker should allow you to, somehow. But I don't have an answer to hand so won't block on it.

Regarding test vs debug code, yeah... Maybe it would be a good idea to not mention the trampoline pointer (mir_connect_impl) in any header at all. Just extern it manually when you need it. That would be better hidden.

[Daniel van Vugt (vanvugt)]

Verified bug 1220443 is fixed.

We can argue over the details later.

review: Approve

[Kevin DuBois (kdub)]

switched to a more hidden extern (was a good idea) so no public-facing headers mention the override. Added a TODO to make a more comprehensive solution to stubbing client functions out

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1064
http://jenkins.qa.ubuntu.com/job/mir-ci/1470/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1987
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1872
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/708
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/708/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I don't like adding implementation to the TestingClientConfiguration interface. The test framework still only uses the exec() function.

Rather than having the tests call functions to change use of connect functions I'd rather see them using an alternative DefaultTestingClientConfiguration/DummyGraphicsTestingClientConfiguration.

I'm not sure that sharing a single TestingServerConfiguration across all tests is a good idea. It shouldn't be a problem (as it is only used in the separate server processes) but it introduces potential for state to leak from one test to another.

review: Needs Fixing

[Robert Carr (robertcarr)]

It looks ok. I think overriding mir_connect is pretty counterintuitive to me though. I wonder if this calls for a 'mir_init' type function which can handle command line arguments, environment, etc, in order to set up client configuration.

[Robert Carr (robertcarr)]

review: Abstain

=== modified file 'include/test/mir_test_framework/display_server_test_fixture.h'
--- include/test/mir_test_framework/display_server_test_fixture.h	2013-08-28 03:41:48 +0000
+++ include/test/mir_test_framework/display_server_test_fixture.h	2013-09-11 21:25:43 +0000
@@ -22,11 +22,14 @@
 #include "mir_test_framework/process.h"
 
 #include "mir_test_framework/testing_process_manager.h"
+#include "mir_test_framework/testing_client_configuration.h"
 #include <gtest/gtest.h>
 
 #include <memory>
 #include <functional>
 
+#include "mir_toolkit/mir_client_library_debug.h"
+
 namespace mir_test_framework
 {
 using namespace ::mir;
@@ -46,6 +49,7 @@
 
 private:
     static TestingProcessManager process_manager;
+    static TestingServerConfiguration default_parameters;
 
     virtual void TearDown();
 };
@@ -73,6 +77,7 @@
     virtual void TearDown();
 
 private:
+    std::shared_ptr<mir::options::Option> server_options;
     TestingProcessManager process_manager;
 };
 

=== added file 'include/test/mir_test_framework/testing_client_configuration.h'
--- include/test/mir_test_framework/testing_client_configuration.h	1970-01-01 00:00:00 +0000
+++ include/test/mir_test_framework/testing_client_configuration.h	2013-09-11 21:25:43 +0000
@@ -0,0 +1,57 @@
+/*
+ * Copyright © 2013 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: Kevin DuBois <[e-mail]>
+ */
+
+#ifndef MIR_TESTING_CLIENT_CONFIGURATION
+#define MIR_TESTING_CLIENT_CONFIGURATION
+
+#include "mir_toolkit/mir_client_library_debug.h"
+#include <memory>
+
+namespace mir
+{
+namespace options
+{
+class Option;
+}
+}
+
+namespace mir_test_framework
+{
+
+struct TestingClientConfiguration
+{
+    TestingClientConfiguration();
+    virtual ~TestingClientConfiguration();
+
+    // Code to run in client process
+    virtual void exec() = 0;
+
+    void set_client_configuration(std::shared_ptr<mir::options::Option> const& options);
+
+    //force client configuration to use default connect and disconnect functions
+    void use_default_connect_functions();
+
+private:
+    MirWaitHandle* (*default_mir_connect_impl)
+        (char const *server, char const *app_name, mir_connected_callback callback, void *context);
+
+    void (*default_mir_connection_release_impl) (MirConnection *connection);
+};
+
+}
+#endif /* MIR_TESTING_CLIENT_CONFIGURATION */

=== modified file 'include/test/mir_test_framework/testing_process_manager.h'
--- include/test/mir_test_framework/testing_process_manager.h	2013-08-28 03:41:48 +0000
+++ include/test/mir_test_framework/testing_process_manager.h	2013-09-11 21:25:43 +0000
@@ -24,6 +24,7 @@
 #include "mir_test_framework/process.h"
 
 #include "mir_test_framework/testing_server_configuration.h"
+#include "mir_test_framework/testing_client_configuration.h"
 
 #include <memory>
 #include <list>
@@ -38,13 +39,6 @@
 {
 using namespace mir;
 
-struct TestingClientConfiguration
-{
-    virtual ~TestingClientConfiguration() = default;
-    // Code to run in client process
-    virtual void exec() = 0;
-};
-
 
 class TestingProcessManager
 {

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-08-28 03:41:48 +0000
+++ src/client/mir_client_library.cpp	2013-09-11 21:25:43 +0000
@@ -77,7 +77,8 @@
 
 }
 
-MirWaitHandle* mir_connect(char const* socket_file, char const* name, mir_connected_callback callback, void * context)
+MirWaitHandle* mir_default_connect(
+    char const* socket_file, char const* name, mir_connected_callback callback, void * context)
 {
 
     try
@@ -93,7 +94,7 @@
             else
                 sock = mir::default_server_socket;
         }
- 
+
         mcl::DefaultConnectionConfiguration conf{sock};
 
         MirConnection* connection = new MirConnection(conf);
@@ -110,6 +111,42 @@
     }
 }
 
+
+void mir_default_connection_release(MirConnection * connection)
+{
+    if (!error_connections.contains(connection))
+    {
+        auto wait_handle = connection->disconnect();
+        wait_handle->wait_for_all();
+    }
+    else
+    {
+        error_connections.remove(connection);
+    }
+
+    delete connection;
+}
+
+//mir_connect and mir_connection_release can be overridden by test code that sets these function
+//pointers to do things like stub out the graphics drivers or change the connection configuration.
+
+//TODO: we could have a more comprehensive solution that allows us to substitute any of the functions
+//for test purposes, not just the connect functions
+MirWaitHandle* (*mir_connect_impl)(
+    char const *server, char const *app_name,
+    mir_connected_callback callback, void *context) = mir_default_connect;
+void (*mir_connection_release_impl) (MirConnection *connection) = mir_default_connection_release;
+
+MirWaitHandle* mir_connect(char const* socket_file, char const* name, mir_connected_callback callback, void * context)
+{
+    return mir_connect_impl(socket_file, name, callback, context);
+}
+
+void mir_connection_release(MirConnection *connection)
+{
+    return mir_connection_release_impl(connection);
+}
+
 MirConnection *mir_connect_sync(char const *server,
                                              char const *app_name)
 {
@@ -131,21 +168,6 @@
     return connection->get_error_message();
 }
 
-void mir_connection_release(MirConnection * connection)
-{
-    if (!error_connections.contains(connection))
-    {
-        auto wait_handle = connection->disconnect();
-        wait_handle->wait_for_all();
-    }
-    else
-    {
-        error_connections.remove(connection);
-    }
-
-    delete connection;
-}
-
 MirEGLNativeDisplayType mir_connection_get_egl_native_display(MirConnection *connection)
 {
     return connection->egl_native_display();

=== modified file 'tests/acceptance-tests/test_client_authorization.cpp'
--- tests/acceptance-tests/test_client_authorization.cpp	2013-08-28 03:41:48 +0000
+++ tests/acceptance-tests/test_client_authorization.cpp	2013-09-11 21:25:43 +0000
@@ -232,6 +232,9 @@
 
         void exec() override
         {
+            //we are testing the connect function itself, so force using default
+            use_default_connect_functions();
+
             pid_t client_pid = getpid();
             shared_region->post_client_process_pid(client_pid);
 

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2013-09-02 09:54:31 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2013-09-11 21:25:43 +0000
@@ -748,6 +748,9 @@
     {
         void exec()
         {
+            //we are testing the connect function itself, so force using default
+            use_default_connect_functions();
+
             mir_wait_for(mir_connect("garbage", __PRETTY_FUNCTION__, connection_callback, this));
             ASSERT_TRUE(connection != NULL);
 
@@ -771,6 +774,9 @@
     {
         void exec()
         {
+            //we are testing the connect function itself, so force using default
+            use_default_connect_functions();
+
             mir_wait_for(mir_connect("garbage", __PRETTY_FUNCTION__, connection_callback, this));
 
             MirSurfaceParameters const request_params =

=== modified file 'tests/mir_test_framework/CMakeLists.txt'
--- tests/mir_test_framework/CMakeLists.txt	2013-08-28 03:41:48 +0000
+++ tests/mir_test_framework/CMakeLists.txt	2013-09-11 21:25:43 +0000
@@ -13,6 +13,7 @@
   testing_server_options.cpp
   input_testing_server_options.cpp
   testing_process_manager.cpp
+  testing_client_options.cpp
   display_server_test_fixture.cpp
   process.cpp
 )

=== modified file 'tests/mir_test_framework/display_server_test_fixture.cpp'
--- tests/mir_test_framework/display_server_test_fixture.cpp	2013-08-28 03:41:48 +0000
+++ tests/mir_test_framework/display_server_test_fixture.cpp	2013-09-11 21:25:43 +0000
@@ -17,20 +17,22 @@
  */
 
 #include "mir_test_framework/display_server_test_fixture.h"
+#include "mir_test_framework/testing_client_configuration.h"
 
 namespace mc = mir::compositor;
-
-mir_test_framework::TestingProcessManager mir_test_framework::DefaultDisplayServerTestFixture::process_manager;
-
+namespace mtf = mir_test_framework;
+
+mtf::TestingProcessManager mir_test_framework::DefaultDisplayServerTestFixture::process_manager;
+mtf::TestingServerConfiguration mir_test_framework::DefaultDisplayServerTestFixture::default_parameters;
 
 void DefaultDisplayServerTestFixture::launch_client_process(TestingClientConfiguration& config)
 {
+    config.set_client_configuration(default_parameters.the_options());
     process_manager.launch_client_process(config);
 }
 
 void DefaultDisplayServerTestFixture::SetUpTestCase()
 {
-    TestingServerConfiguration default_parameters;
     process_manager.launch_server_process(default_parameters);
 }
 
@@ -54,11 +56,13 @@
 
 void BespokeDisplayServerTestFixture::launch_server_process(TestingServerConfiguration& functor)
 {
+    server_options = functor.the_options();
     process_manager.launch_server_process(functor);
 }
 
 void BespokeDisplayServerTestFixture::launch_client_process(TestingClientConfiguration& config)
 {
+    config.set_client_configuration(server_options);
     process_manager.launch_client_process(config);
 }
 

=== added file 'tests/mir_test_framework/testing_client_options.cpp'
--- tests/mir_test_framework/testing_client_options.cpp	1970-01-01 00:00:00 +0000
+++ tests/mir_test_framework/testing_client_options.cpp	2013-09-11 21:25:43 +0000
@@ -0,0 +1,181 @@
+/*
+ * Copyright © 2013 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: Kevin DuBois <[e-mail]>
+ */
+
+#include "mir_test_framework/testing_client_configuration.h"
+#include "mir/options/program_option.h"
+#include "src/client/default_connection_configuration.h"
+#include "src/client/client_platform_factory.h"
+#include "src/client/client_buffer_factory.h"
+#include "src/client/client_buffer.h"
+#include "src/client/client_platform.h"
+#include "src/client/mir_connection.h"
+
+namespace mcl = mir::client;
+namespace mtf=mir_test_framework;
+namespace geom = mir::geometry;
+
+/* 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
+ */
+extern MirWaitHandle* (*mir_connect_impl)(
+    char const *server,
+    char const *app_name,
+    mir_connected_callback callback,
+    void *context);
+extern void (*mir_connection_release_impl) (MirConnection *connection);
+
+namespace
+{
+class StubClientBuffer : public mcl::ClientBuffer
+{
+    std::shared_ptr<mcl::MemoryRegion> secure_for_cpu_write()
+    {
+        return nullptr;
+    }
+
+    geom::Size size() const
+    {
+        return geom::Size{};
+    }
+
+    geom::Stride stride() const
+    {
+        return geom::Stride{};
+    }
+
+    geom::PixelFormat pixel_format() const
+    {
+        return geom::PixelFormat::abgr_8888;
+    }
+    
+    uint32_t age() const
+    {
+        return 0;
+    }
+    void increment_age()
+    {
+    }
+    void mark_as_submitted()
+    {
+    }
+    std::shared_ptr<MirNativeBuffer> native_buffer_handle() const
+    {
+        return nullptr;
+    }
+};
+
+struct StubClientBufferFactory : public mcl::ClientBufferFactory
+{
+    std::shared_ptr<mcl::ClientBuffer> create_buffer(std::shared_ptr<MirBufferPackage> const&,
+                                                     geom::Size, geom::PixelFormat)
+    {
+        return std::make_shared<StubClientBuffer>();
+    }
+};
+
+struct StubClientPlatform : public mcl::ClientPlatform
+{
+    MirPlatformType platform_type() const
+    {
+        return mir_platform_type_gbm;
+    }
+
+    std::shared_ptr<mcl::ClientBufferFactory> create_buffer_factory()
+    {
+        return std::make_shared<StubClientBufferFactory>();
+    }
+
+    std::shared_ptr<EGLNativeWindowType> create_egl_native_window(mcl::ClientSurface*)
+    {
+        auto fake_window = reinterpret_cast<EGLNativeWindowType>(0x12345678);
+        return std::make_shared<EGLNativeWindowType>(fake_window);
+    }
+
+    std::shared_ptr<EGLNativeDisplayType> create_egl_native_display()
+    {
+        auto fake_display = reinterpret_cast<EGLNativeDisplayType>(0x12345678);
+        return std::make_shared<EGLNativeDisplayType>(fake_display);
+    }
+};
+
+struct StubClientPlatformFactory : public mcl::ClientPlatformFactory
+{
+    std::shared_ptr<mcl::ClientPlatform> create_client_platform(mcl::ClientContext*)
+    {
+        return std::make_shared<StubClientPlatform>();
+    }
+};
+
+struct StubConnectionConfiguration : public mcl::DefaultConnectionConfiguration
+{
+    StubConnectionConfiguration(std::string const& socket_file)
+        : DefaultConnectionConfiguration(socket_file)
+    {
+    }
+
+    std::shared_ptr<mcl::ClientPlatformFactory> the_client_platform_factory() override
+    {
+        return std::make_shared<StubClientPlatformFactory>();
+    }
+};
+
+MirWaitHandle* mir_connect_test_override(
+    char const *socket_file,
+    char const *app_name,
+    mir_connected_callback callback,
+    void *context)
+{
+    StubConnectionConfiguration conf(socket_file);
+    auto connection = new MirConnection(conf);
+    return connection->connect(app_name, callback, context);
+}
+
+void mir_connection_release_override(MirConnection *connection)
+{
+    auto wait_handle = connection->disconnect();
+    wait_handle->wait_for_all();
+    delete connection;
+}
+
+}
+
+mtf::TestingClientConfiguration::TestingClientConfiguration()
+    : default_mir_connect_impl(mir_connect_impl),
+      default_mir_connection_release_impl(mir_connection_release_impl)
+{
+}
+
+void mtf::TestingClientConfiguration::use_default_connect_functions()
+{
+    mir_connect_impl = default_mir_connect_impl;
+    mir_connection_release_impl = default_mir_connection_release_impl;
+}
+
+void mtf::TestingClientConfiguration::set_client_configuration(std::shared_ptr<mir::options::Option> const& options)
+{
+    if (!options->get("tests-use-real-graphics", false))
+    {
+        mir_connect_impl = mir_connect_test_override;
+        mir_connection_release_impl = mir_connection_release_override;
+    }
+}
+
+mtf::TestingClientConfiguration::~TestingClientConfiguration()
+{
+    use_default_connect_functions();
+}