Mir

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

Proposed by Robert Carr
Status: Merged
Merged at revision: 2157
Proposed branch: lp://qastaging/~mir-team/mir/port-uncooperative-tests-to-surface-spec
Merge into: lp://qastaging/mir
Prerequisite: lp://qastaging/~mir-team/mir/port-cooperative-tests-to-surface-spec
Diff against target: 124 lines (+25/-27)
3 files modified
src/client/mir_surface.cpp (+21/-2)
src/client/rpc/mir_protobuf_rpc_channel.cpp (+0/-2)
tests/acceptance-tests/test_server_disconnect.cpp (+4/-23)
To merge this branch: bzr merge lp://qastaging/~mir-team/mir/port-uncooperative-tests-to-surface-spec
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Needs Fixing
Alan Griffiths Approve
Cemil Azizoglu (community) Approve
Review via email: mp+244057@code.qastaging.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Chris Halse Rogers (raof) wrote :

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

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

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
Revision history for this message
Robert Carr (robertcarr) wrote :

>> 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?

Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

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

Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

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

review: Approve
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> >> 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
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

$ 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
Revision history for this message
Robert Carr (robertcarr) wrote :

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.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

OK

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches