Mir

Merge lp://qastaging/~mir-team/mir/only-one-way-to-run-the-tests into lp://qastaging/mir

only-one-way-to-run-the-tests
Merge into development-branch

Proposed by Chris Halse Rogers on 2015-04-07

Status:	Work in progress
Proposed branch:	lp://qastaging/~mir-team/mir/only-one-way-to-run-the-tests
Merge into:	lp://qastaging/mir
Diff against target:	617 lines (+43/-530) 3 files modified cmake/MirCommon.cmake (+43/-115) cmake/src/mir/CMakeLists.txt (+0/-10) cmake/src/mir/mir_discover_gtest_tests.cpp (+0/-405)
To merge this branch:	bzr merge lp://qastaging/~mir-team/mir/only-one-way-to-run-the-tests
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Alan Griffiths			Needs Resubmitting on 2015-04-13
PS Jenkins bot (community)	continuous-integration		Approve on 2015-04-10
Alberto Aguirre (community)			Needs Information on 2015-04-09
Alexandros Frantzis (community)		2015-04-07	Needs Information on 2015-04-07
Review via email: mp+255326@code.qastaging.launchpad.net

Commit message

Drop DISABLE_GTEST_TEST_DISCOVERY option.

This was used to not run the test executables to discover their tests when
cross-building. But since we're cross-building we're not going to run the tests
*anyway*, so just don't add the test targets instead.

Description of the change

Drop DISABLE_GTEST_TEST_DISCOVERY option.

Whenever we need to modify the environment the tests run in this gets in our way. I don't know any reason for itother than “you can't run test discovery while cross-compiling”, and that can be better served by just not running test discovery while cross-compiling :).

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-04-07:

We are currently using this option in CI runs to reduce the test runtime [1]. This has a significant effect, since it avoids the startup cost of new processes which is very high with valgrind (especially on armhf).

Do we actually need test discovery any more? IIRC, its goal was to get more information when tests failed in CI, but we can get enough information by using ctest in verbose mode instead of using test discovery, which is exactly what we have been doing for a long time in CI anyway.

I propose we move in the opposite direction and remove the test discovery mode.

Needs discussion/Disapprove

[1] https://bazaar.launchpad.net/~private-ps-quality-team/pbuilderjenkins/trunk/view/head:/hooks/H15enable_testing.in

review: Needs Information

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-07:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-07:

> We are currently using this option in CI runs to reduce the test runtime [1].
> This has a significant effect, since it avoids the startup cost of new
> processes which is very high with valgrind (especially on armhf).
>
> Do we actually need test discovery any more? IIRC, its goal was to get more
> information when tests failed in CI, but we can get enough information by
> using ctest in verbose mode instead of using test discovery, which is exactly
> what we have been doing for a long time in CI anyway.
>
> I propose we move in the opposite direction and remove the test discovery
> mode.

That makes sense to me.

review: Needs Information

lp://qastaging/~mir-team/mir/only-one-way-to-run-the-tests updated on 2015-04-07

2457. By Alan Griffiths on 2015-04-07

examples: tweak to the way decorations are created.

Approved by PS Jenkins bot, Alexandros Frantzis, Kevin DuBois.

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-04-07:

> We are currently using this option in CI runs to reduce the test runtime [1].
> This has a significant effect, since it avoids the startup cost of new processes
> which is very high with valgrind (especially on armhf).

A case in point, from https://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/buildTimeTrend:

Success #1911 1 hr 15 min
Success #1910 1 hr 25 min
Success #1909 3 hr 30 min <== This MP
Success #1908 1 hr 24 min
Success #1907 1 hr 23 min
Success #1906 1 hr 16 min
Success #1905 1 hr 26 min

lp://qastaging/~mir-team/mir/only-one-way-to-run-the-tests updated on 2015-04-07

2458. By Alexandros Frantzis on 2015-04-07

tests: Fix failure when running SimpleDispatchThreadTest.doesnt_call_dispatch_after_first_false_return repeatedly (LP: #1440005). Fixes: https://bugs.launchpad.net/bugs/1440005.

Approved by Chris Halse Rogers, Alan Griffiths, Kevin DuBois, PS Jenkins bot.

2459. By Alexandros Frantzis on 2015-04-07

server,tests: Ensure compositor is fully started before allowing client connections (LP: #1440088). Fixes: https://bugs.launchpad.net/bugs/1440088.

Approved by Alan Griffiths, PS Jenkins bot, Kevin DuBois.

2460. By Kevin DuBois on 2015-04-07

Rename some classes in graphics/nested:
mir::graphics::nested::NestedDisplay -> mir:graphics::nested::Display
mir::graphics::nested::NestedDisplayOutput -> mir:graphics::nested::DisplayBuffer
and split HostSurface to its own header file.

Approved by PS Jenkins bot, Alan Griffiths, Alexandros Frantzis.

2461. By Alexandros Frantzis on 2015-04-07

common: Fix SimpleDispatchThreadTest signal interruption bug (LP: #1439719). Fixes: https://bugs.launchpad.net/bugs/1439719.

Approved by PS Jenkins bot, Alan Griffiths, Chris Halse Rogers, Robert Carr.

2462. By Alan Griffiths on 2015-04-07

frontend, shell: rewire the surface modification requests through window management.

Approved by Alexandros Frantzis, Kevin DuBois, Robert Carr, PS Jenkins bot.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-04-07:

Aha! A reason why we have the no-test-discovery mode! We were wondering in the 2-3rds call.

I'd be slightly sad to lose test discovery - it means you can easily run the entire testsuite in parallel. We could fix this in other ways, though.

lp://qastaging/~mir-team/mir/only-one-way-to-run-the-tests updated on 2015-04-08

2463. By Chris Halse Rogers on 2015-04-08

Drop GTest test discovery.

There's no particularly good reason to have two different ways of running the testsuite.
Drop the one that isn't 3x slower on armhf CI due to valgrind process startup inefficiencies.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-08:

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

review: Approve (continuous-integration)

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-04-09:

===
83 - if(cmake_build_type_lower MATCHES "threadsanitizer")
84 - find_program(LLVM_SYMBOLIZER llvm-symbolizer-3.6)
85 - if (LLVM_SYMBOLIZER)
86 - set(TSAN_EXTRA_OPTIONS "external_symbolizer_path=${LLVM_SYMBOLIZER}")
87 - endif()
88 - list(APPEND EXTRA_ENV_FLAGS "--add-environment" "TSAN_OPTIONS=suppressions=${CMAKE_SOURCE_DIR}/tools/tsan-suppressions second_deadlock_stack=1 halt_on_error=1 history_size=7 ${TSAN_EXTRA_OPTIONS}")
89 - # TSan does not support multi-threaded fork
90 - # TSan may open fds so "surface_creation_does_not_leak_fds" will not work as written
91 - # TSan deadlocks when running StreamTransportTest/0.SendsFullMessagesWhenInterrupted - disable it until understood
92 - set(EXCLUDED_TESTS "UnresponsiveClient.does_not_hang_server:DemoInProcessServerWithStubClientPlatform.surface_creation_does_not_leak_fds:StreamTransportTest/0.SendsFullMessagesWhenInterrupted")
93 - endif()
===

I was planning to use that to run the test suite for ThreadSanitizer builds in CI.

Is there a compelling reason to remove the test discovery?

review: Needs Information

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-04-10:

Oh, I just accidentally removed that. I'll reinstate it for the non-discovery mechanism.

There's not a *huge* reason to remove test discovery, but it's not how (most) tests are run in CI, and having two test codepaths makes changing the test environment more annoying - for example, as is, ThreadSanitizer builds will fail without test discovery, because you didn't add the EXCLUDED_TESTS logic to both test-running codepaths :)

lp://qastaging/~mir-team/mir/only-one-way-to-run-the-tests updated on 2015-04-10

2464. By Chris Halse Rogers on 2015-04-10: Re-add the mistakenly-dropped TSan handling
2465. By Chris Halse Rogers on 2015-04-10: Remove some unnecessary Valgrind fiddling

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-04-10:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-04-13:

> I'd be slightly sad to lose test discovery - it means you can easily run the
> entire testsuite in parallel. We could fix this in other ways, though.

This seems to be a useful feature and I'm not convinced there's a net benefit to dropping it until we "fix this in other ways".

review: Needs Resubmitting

Unmerged revisions

2465. By Chris Halse Rogers on 2015-04-10

Remove some unnecessary Valgrind fiddling

2464. By Chris Halse Rogers on 2015-04-10

Re-add the mistakenly-dropped TSan handling

2463. By Chris Halse Rogers on 2015-04-08

Drop GTest test discovery.

There's no particularly good reason to have two different ways of running the testsuite.
Drop the one that isn't 3x slower on armhf CI due to valgrind process startup inefficiencies.