Mir

Merge lp://qastaging/~vanvugt/mir/fix-1342092 into lp://qastaging/mir

fix-1342092
Merge into development-branch

Proposed by Daniel van Vugt on 2016-03-18

Status:

Merged

Approved by:

Alan Griffiths on 2016-03-21

Approved revision:

no longer in the source branch.

Merged at revision:

3412

Proposed branch:

lp://qastaging/~vanvugt/mir/fix-1342092

Merge into:

lp://qastaging/mir

Diff against target:

357 lines (+108/-25)

9 files modified

include/test/mir_test_framework/interprocess_client_server_test.h (+2/-0)
tests/acceptance-tests/server_signal_handling.cpp (+8/-2)
tests/acceptance-tests/test_server_shutdown.cpp (+5/-5)
tests/acceptance-tests/throwback/test_client_library_errors.cpp (+14/-6)
tests/include/mir/test/death.h (+49/-0)
tests/mir_test_framework/interprocess_client_server_test.cpp (+11/-0)
tests/unit-tests/dispatch/test_threaded_dispatcher.cpp (+7/-3)
tests/unit-tests/test_fatal.cpp (+7/-5)
tests/unit-tests/test_udev_wrapper.cpp (+5/-4)

To merge this branch:

bzr merge lp://qastaging/~vanvugt/mir/fix-1342092

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration	2016-03-18	Approve on 2016-03-21
Alexandros Frantzis (community)		2016-03-18	Approve on 2016-03-21
Alan Griffiths		2016-03-18	Approve on 2016-03-18
Mir development team		2016-03-18	Pending
Kevin DuBois		2016-03-18	Pending
Chris Halse Rogers		2016-03-18	Pending
Review via email: mp+289454@code.qastaging.launchpad.net

Commit message

Avoid dumping core from death tests that are expected to die
(LP: #1342092)

But try to remember to only disable cores in child processes, so that
faulty tests can still dump core.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-18:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/616/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-03-18:

Not related:
[ FAILED ] NestedServer.named_cursor_image_changes_are_forwarded_to_host

And also not related:
std::exception::what: Failed to read from device: /dev/random after: 30 seconds
(LP: #1541188)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-18:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/618/rebuild

review: Approve (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-18:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/619/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-18:

Nits, (could be addressed later)

=== added file 'tests/include/mir/test/death.h'

Might be a useful addition to mirtest-dev (i.e. tests/include/mir/test/death.h => include/mir/test/death.h)

~~~~

+#include <sys/resource.h>
...
+inline void disable_core_dump()
+{
+ struct rlimit zeroes{0,0};
+ setrlimit(RLIMIT_CORE, &zeroes);
+}

Moving this function out of line would avoid adding the above #include to all TUs that use this header.

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-03-21:

1. Yeah I considered that, but our public APIs are too large already. Better to keep things private till they need to be public.

2. I did also consider non-inlining that function. Although the inline approach was initially just for a quick proof of concept, I think it's probably more efficient to keep it that way. If you consider the full namespace 'mir::test::disable_core_dump' then the symbol name is probably longer than the machine code of the function. So more efficient to inline it with no function call/linkage.
I would also argue that it's effectively a single-line function and as such should even not exist at all -- callers perhaps should just setrlimit() themselves.

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2016-03-21:

> 2. I did also consider non-inlining that function. Although the inline approach was initially
> just for a quick proof of concept, I think it's probably more efficient to keep it that way

I don't think we should care about this level of efficiency in the tests. I would also prefer a non-inlined version, but not enough to block.

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) on 2016-03-21:

review: Approve (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

to all changes:

Daniel van Vugt

Emanuele Antonio Faraone

Gerry Boland

Mir development team

Mir

Merge lp://qastaging/~vanvugt/mir/fix-1342092 into lp://qastaging/mir

Commit message

Description of the change

Preview Diff

Subscribers