Merge lp:~mir-team/mir/port-uncooperative-tests-to-surface-spec into lp:mir

[Robert Carr]

Port uncooperative tests to surface spec by first fixing https://bugs.launchpad.net/mir/+bug/1394873

Port uncooperative tests to surface spec by first fixing https://bugs.launchpad.net/mir/+bug/1394873

[Chris Halse Rogers (raof)]

I'm not sure that's the right fix? Maybe it's the best we can do given protobuf's callbacks?

[Alan Griffiths (alan-griffiths)]

19 - send_message(invocation, invocation, fds);
20 + try
21 + {
22 + send_message(invocation, invocation, fds);
23 + }
24 + // In general we propagate this exception up to the client
25 + // library so that it may invoke appropriate error callbacks,
26 + // etc...however in the case of disconnect the notify_disconnect
27 + // codepath will take care of this and we need to surpress the
28 + // exception here. (lp:1394873)
29 + catch (...)
30 + {
31 + if (!disconnected.load())
32 + throw;
33 + }

This is not really anything to do with the stated reason for this MP is it?

review: Needs Information

[Robert Carr (robertcarr)]

>> his is not really anything to do with the stated reason for this MP is it?

https://code.launchpad.net/bugs/1394873

Is it the branch title you'd like me to change?

[Cemil Azizoglu (cemil-azizoglu)]

So, fix + port. Probably wanna separate the fix out in its own MP.

For the ports, same comments apply :
https://code.launchpad.net/~mir-team/mir/port-cooperative-tests-to-surface-spec/+merge/244056/comments/601848

[Cemil Azizoglu (cemil-azizoglu)]

Approving this. Port is ok (as mentioned in the other MP).

review: Approve

[Alan Griffiths (alan-griffiths)]

> >> his is not really anything to do with the stated reason for this MP is it?
>
> https://code.launchpad.net/bugs/1394873
>
> Is it the branch title you'd like me to change?

No, just confirming my understanding

review: Approve

[Alan Griffiths (alan-griffiths)]

$ bin/mir_unit_tests --gtest_filter=MirProtobufRpcChannelTest.*
Running main() from command_line_server_configuration.cpp
Note: Google Test filter = MirProtobufRpcChannelTest.*
[==========] Running 8 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 8 tests from MirProtobufRpcChannelTest
[ RUN ] MirProtobufRpcChannelTest.ReadsFullMessages
[ OK ] MirProtobufRpcChannelTest.ReadsFullMessages (0 ms)
[ RUN ] MirProtobufRpcChannelTest.ReadsAllQueuedMessages
[ OK ] MirProtobufRpcChannelTest.ReadsAllQueuedMessages (0 ms)
[ RUN ] MirProtobufRpcChannelTest.SendsMessagesAtomically
[ OK ] MirProtobufRpcChannelTest.SendsMessagesAtomically (1 ms)
[ RUN ] MirProtobufRpcChannelTest.SetsCorrectSizeWhenSendingMessage
[ OK ] MirProtobufRpcChannelTest.SetsCorrectSizeWhenSendingMessage (0 ms)
[ RUN ] MirProtobufRpcChannelTest.ReadsFds
[ OK ] MirProtobufRpcChannelTest.ReadsFds (1 ms)
[ RUN ] MirProtobufRpcChannelTest.NotifiesOfDisconnectOnWriteError
/home/alan/display_server/mir4/tests/unit-tests/client/test_protobuf_rpc_channel.cpp:321: Failure
Expected: channel_user.exchange_buffer(nullptr, &request, &reply, google::protobuf::NewCallback([](){})) throws an exception of type std::runtime_error.
  Actual: it throws nothing.
[ FAILED ] MirProtobufRpcChannelTest.NotifiesOfDisconnectOnWriteError (0 ms)
[ RUN ] MirProtobufRpcChannelTest.ForwardsDisconnectNotification
[ OK ] MirProtobufRpcChannelTest.ForwardsDisconnectNotification (0 ms)
[ RUN ] MirProtobufRpcChannelTest.NotifiesOfDisconnectOnlyOnce
/home/alan/display_server/mir4/tests/unit-tests/client/test_protobuf_rpc_channel.cpp:382: Failure
Expected: channel_user.exchange_buffer(nullptr, &request, &reply, google::protobuf::NewCallback([](){})) throws an exception of type std::runtime_error.
  Actual: it throws nothing.
[ FAILED ] MirProtobufRpcChannelTest.NotifiesOfDisconnectOnlyOnce (1 ms)
[----------] 8 tests from MirProtobufRpcChannelTest (3 ms total)

[----------] Global test environment tear-down
[==========] 8 tests from 1 test case ran. (4 ms total)
[ PASSED ] 6 tests.
[ FAILED ] 2 tests, listed below:
[ FAILED ] MirProtobufRpcChannelTest.NotifiesOfDisconnectOnWriteError
[ FAILED ] MirProtobufRpcChannelTest.NotifiesOfDisconnectOnlyOnce

 2 FAILED TESTS

review: Needs Fixing

[Robert Carr (robertcarr)]

I think this is the right fix now...(in r2133).

In the case that send_message throws, the MirSurface constructor will now catch it and convert it in to an error. In this case the client library wont catch an exception and wont construct the duplicate error object.

The exception catch + error object creation is left in for cases where MirSurface constructor really does throw (e.g. as in the test which simulates BufferFactory throw failure).

It was necessary to make some changes in MirSurface::created to handle the pending call completion for the surface which never received a response.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2133
http://jenkins.qa.ubuntu.com/job/mir-ci/2338/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/445
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/445
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/419/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/335/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/419/console

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2135
http://jenkins.qa.ubuntu.com/job/mir-ci/2401/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/512
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/512
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/486/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/398/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/486
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/486/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3669/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16562

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

review: Needs Fixing (continuous-integration)

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2014-12-09 03:14:55 +0000
+++ src/client/mir_surface.cpp	2014-12-15 07:36:03 +0000
@@ -127,7 +127,15 @@
     message.set_output_id(params.output_id);
 
     create_wait_handle.expect_result();
-    server.create_surface(0, &message, &surface, gp::NewCallback(this, &MirSurface::created, callback, context));
+    try 
+    {
+        server.create_surface(0, &message, &surface, gp::NewCallback(this, &MirSurface::created, callback, context));
+    }
+    catch (std::exception const& ex)
+    {
+        surface.set_error(std::string{"Error invoking create surface: "} +
+                          boost::diagnostic_information(ex));
+    }
 
     std::lock_guard<decltype(handle_mutex)> lock(handle_mutex);
     valid_surfaces.insert(this);
@@ -283,8 +291,19 @@
 
 void MirSurface::created(mir_surface_callback callback, void * context)
 {
+    {
+    std::lock_guard<decltype(mutex)> lock(mutex);
+    if (!surface.has_id())
+    {
+        if (!surface.has_error())
+            surface.set_error("Error processing surface create response, no ID (disconnected?)");
+        callback(this, context);
+        create_wait_handle.result_received();
+        return;
+    }
+    }
+
     auto platform = connection->get_client_platform();
-
     try
     {
         {

=== modified file 'src/client/rpc/mir_protobuf_rpc_channel.cpp'
--- src/client/rpc/mir_protobuf_rpc_channel.cpp	2014-12-15 03:17:26 +0000
+++ src/client/rpc/mir_protobuf_rpc_channel.cpp	2014-12-15 07:36:03 +0000
@@ -82,7 +82,6 @@
     pending_calls.force_completion();
 }
 
-
 template<class MessageType>
 void mclr::MirProtobufRpcChannel::receive_any_file_descriptors_for(MessageType* response)
 {
@@ -189,7 +188,6 @@
     std::shared_ptr<google::protobuf::Closure> callback(
         google::protobuf::NewPermanentCallback(this, &MirProtobufRpcChannel::receive_file_descriptors, response, complete));
 
-    // Only save details after serialization succeeds
     pending_calls.save_completion_details(invocation, response, callback);
 
     send_message(invocation, invocation, fds);

=== modified file 'tests/acceptance-tests/test_server_disconnect.cpp'
--- tests/acceptance-tests/test_server_disconnect.cpp	2014-12-08 04:03:47 +0000
+++ tests/acceptance-tests/test_server_disconnect.cpp	2014-12-15 07:36:03 +0000
@@ -21,6 +21,7 @@
 #include "mir_test_framework/interprocess_client_server_test.h"
 #include "mir_test_framework/cross_process_sync.h"
 #include "mir_test_framework/process.h"
+#include "mir_test_framework/any_surface.h"
 #include "mir_test/cross_process_action.h"
 
 #include <gtest/gtest.h>
@@ -63,15 +64,11 @@
 
     auto const client = new_client_process([&]
         {
-            static MirSurfaceParameters const params =
-                { __PRETTY_FUNCTION__, 33, 45, mir_pixel_format_abgr_8888,
-                  mir_buffer_usage_hardware, mir_display_output_id_invalid };
-
             MockEventHandler mock_event_handler;
 
             auto connection = mir_connect_sync(mtf::test_socket_file().c_str() , __PRETTY_FUNCTION__);
             mir_connection_set_lifecycle_event_callback(connection, &MockEventHandler::handle, &mock_event_handler);
-            auto surface = mir_connection_create_surface_sync(connection, &params);
+            auto surface = mtf::make_any_surface(connection);
 
             std::atomic<bool> signalled(false);
 
@@ -125,15 +122,7 @@
             MirSurface* surf;
 
             create_surface.exec([&] {
-                MirSurfaceParameters const parameters =
-                {
-                    __PRETTY_FUNCTION__,
-                    1, 1,
-                    mir_pixel_format_abgr_8888,
-                    mir_buffer_usage_hardware,
-                    mir_display_output_id_invalid
-                };
-                surf = mir_connection_create_surface_sync(connection, &parameters);
+                surf = mtf::make_any_surface(connection);
             });
 
             configure_display.exec([&] {
@@ -178,15 +167,7 @@
 
             create_surface_sync.wait_for_signal_ready_for();
 
-            MirSurfaceParameters const parameters =
-            {
-                __PRETTY_FUNCTION__,
-                1, 1,
-                mir_pixel_format_abgr_8888,
-                mir_buffer_usage_hardware,
-                mir_display_output_id_invalid
-            };
-            mir_connection_create_surface_sync(connection, &parameters);
+            mtf::make_any_surface(connection);
 
             mir_connection_release(connection);
         });