Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add wlcs tests #22

Merged
merged 7 commits into from Nov 14, 2017
Merged

Add wlcs tests #22

merged 7 commits into from Nov 14, 2017

Conversation

RAOF
Copy link
Contributor

@RAOF RAOF commented Nov 10, 2017

This integrates wlcs with the Mir test-suite, gated on MIR_RUN_ACCEPTANCE_TESTS.

It then fixes the bugs exposed!

This embeds libwlcs as a git submodule and adds the ability to get a Wayland client
socket fd from mir::Server so that the wlcs tests can run against Mir.

The wlcs tests currently fail against Mir. Yay for tests!
…xtures.

The Wayland frame events are driven by the surface's buffer being bound, *not* by the buffer
being requested by the compositor.

The wlcs tests now don't hang, and instead fail because Mir isn't protocol-compliant. Yay!
We need to do this eagerly because wl_shm_buffer_end_access(buffer) can attempt to send
a protocol error on buffer when SIGBUS is raised. This has to be done on the Wayland
thread.

Furthermore, wl_shm_buffer_begin_access()/wl_shm_buffer_end_access() need to be called
on the same thread, so we can't just punt wl_shm_buffer_end_access() to the eventloop.

Since we don't want the renderer thread to block waiting on the Wayland eventloop to process
the buffer we eagerly copy and then just upload the previously-copied data on bind().

As an optimisation this should be changed so that the glTexImage2D call is made on
construction, taking the (slow) texture upload off the renderer hotpath.
… configure message.

The bad buffer test causes a protocol error to be raised; this results in the client
being disconnected and all its resources being destroyed.

Since we punt the initial wl_shell_surface_send_configure request out to the eventloop
it might get run after the protocol error causes everything to be torn down, resulting
in a crash.

We probably need to factor this out into a generic event-sender, but for now this
lets us pass the wlcs tests.
It will manage its own licencing, thank you.
@AlanGriffiths
Copy link
Contributor

When I build and run locally I see (once I persuaded make to even run the wlcs test):

The following tests FAILED:
5 - mir_wlcs_tests.BadBufferTest.* (Failed)
6 - mir_wlcs_tests.ClientSurfaceEventsTest.* (Failed)

Both tests segfault!

Not sure what happens in CI - there's no output from the test run.

@AlanGriffiths
Copy link
Contributor

It is always wrong when "(this=0x0"

(gdb) bt
#0 0x00007ffff7b61484 in std::unique_ptr<wlcs::ShmBuffer::Impl, std::default_deletewlcs::ShmBuffer::Impl >::get (this=0x0) at /usr/include/c++/6/bits/unique_ptr.h:308
#1 0x00007ffff7b5ef3e in std::unique_ptr<wlcs::ShmBuffer::Impl, std::default_deletewlcs::ShmBuffer::Impl >::operator-> (this=0x0) at /usr/include/c++/6/bits/unique_ptr.h:302
#2 0x00007ffff7b5be94 in wlcs::ShmBuffer::add_release_listener(std::function<bool ()> const&) (this=0x0,
on_release=...)
at /home/alan/MirServer/mir/tests/acceptance-tests/wayland/wlcs/src/in_process_server.cpp:454
#3 0x00007ffff7b5ca93 in wlcs::Client::Impl::create_visible_surface (this=0x555555ad4d30, client=...,
width=200, height=200)
at /home/alan/MirServer/mir/tests/acceptance-tests/wayland/wlcs/src/in_process_server.cpp:211
#4 0x00007ffff7b5bc00 in wlcs::Client::create_visible_surface (this=0x7fffffffd6b8, width=200, height=200)
at /home/alan/MirServer/mir/tests/acceptance-tests/wayland/wlcs/src/in_process_server.cpp:303
#5 0x00007ffff7b64e15 in BadBufferTest_test_truncated_shm_file_Test::TestBody (this=0x555555acaeb0)
at /home/alan/MirServer/mir/tests/acceptance-tests/wayland/wlcs/tests/test_bad_buffer.cpp:79
#6 0x00007ffff7b9543b in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (
object=0x555555acaeb0, method=&virtual testing::Test::TestBody(),

Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    // It's safe to free the buffer after the release event has been received.
    buffer->add_release_listener([buffer = std::move(buffer)]() { return false; });

But it is not safe to use buffer->add_release_listener when it isn't sequence-before the std::move()

@AlanGriffiths
Copy link
Contributor

OK, I've proposed a fix against wlcs. But that doesn't get everything working.

  1. the wlcs tests seem to only run if -DENABLE_MEMCHECK_OPTION=on (not obvious to me why)
  2. with that option on I get:

$ ctest -V -R .wlcs.
The following tests passed:
mir_wlcs_tests_no_memcheck

50% tests passed, 1 tests failed out of 2

Total Test time (real) = 15.64 sec

The following tests FAILED:
13 - mir_wlcs_tests (Failed)
Errors while running CTest

@AlanGriffiths
Copy link
Contributor

I've pushed the fix for mir_wlcs_tests to wlcs - was just bad setup in mir_integration.cpp

Still need to work out what's happening with getting the tests to actually run

Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, these tests are leaking FDs and fail with ENABLE_MEMCHECK_OPTION=on

@AlanGriffiths
Copy link
Contributor

This is what doesn't work for me:

git clone https://github.com/MirServer/mir
cd mir
git checkout add-wlcs-tests
git submodule update --init --recursive
mkdir build
cd build/
cmake ..
ctest --show-only | grep wlcs
(nothing)

However:

cmake -DENABLE_MEMCHECK_OPTION=on ..
ctest --show-only | grep wlcs
Test #13: mir_wlcs_tests
...

@AlanGriffiths
Copy link
Contributor

anyway, I think we can leave that as an exercise for later.

bors r+

bors bot added a commit that referenced this pull request Nov 14, 2017
22: Add wlcs tests r=AlanGriffiths a=RAOF

This integrates `wlcs` with the Mir test-suite, gated on MIR_RUN_ACCEPTANCE_TESTS.

It then fixes the bugs exposed!
@bors
Copy link
Contributor

bors bot commented Nov 14, 2017

Build succeeded

@bors bors bot merged commit 507ab5c into master Nov 14, 2017
@AlanGriffiths AlanGriffiths deleted the add-wlcs-tests branch November 14, 2017 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants