feature: a test harness for window managers + tests for the MinimalWindowManager #3416

mattkae · 2024-06-06T14:08:32Z

This PR establishes an initial test harness that is suitable for MinimalWindowManager. We might need more harnessing for FloatingWindowManager, but that should come as a follow up if we need it.

What's new?

Built out the WindowManagementTestHarness
Wrote some initial integration tests for the MinimalWindowManager

AlanGriffiths · 2024-06-10T16:15:45Z

@mattkae any idea why "Coverage" failed with:

[ FAILED ] 2 tests, listed below:
[ FAILED ] Process.a_main_fn_is_executed
[ FAILED ] Process.a_successful_exit_function_succeeds

It isn't like it runs different tests?

src/server/symbols.map

AlanGriffiths

Overall, this is the sort of thing I imagined. Not finished looking through the detail yet. But mentioning some nits before my EOD

AlanGriffiths · 2024-06-10T16:40:08Z

tests/include/mir/test/doubles/stub_shell_report.h

+class StubShellReport : public mir::shell::ShellReport
+{
+public:
+    void opened_session(mir::scene::Session const&) override {}
+    void closing_session(mir::scene::Session const&) override {}
+    void created_surface(mir::scene::Session const&, mir::scene::Surface const&) override {}
+    void update_surface(
+    mir::scene::Session const&, mir::scene::Surface const&,
+    mir::shell::SurfaceSpecification const&) override {}
+    void update_surface(
+    mir::scene::Session const&, mir::scene::Surface const&,
+    MirWindowAttrib, int) override {}
+    void destroying_surface(mir::scene::Session const&, mir::scene::Surface const&) override {}
+    void started_prompt_session(mir::scene::PromptSession const&, mir::scene::Session const&) override {}
+    void added_prompt_provider(mir::scene::PromptSession const&, mir::scene::Session const&) override {}
+    void stopping_prompt_session(mir::scene::PromptSession const&) override {}
+    void adding_display(mir::geometry::Rectangle const&) override {}
+    void removing_display(mir::geometry::Rectangle const&) override {}
+    void input_focus_set_to(mir::scene::Session const*, mir::scene::Surface const*) override {}
+    void surfaces_raised(mir::shell::SurfaceSet const&) override {}
+};


This looks very similar to the existing mir::report::null::ShellReport implementation. Do we need both?

tests/integration-tests/CMakeLists.txt

mattkae · 2024-06-10T17:31:30Z

@mattkae any idea why "Coverage" failed with:
[ FAILED ] 2 tests, listed below:
[ FAILED ] Process.a_main_fn_is_executed
[ FAILED ] Process.a_successful_exit_function_succeeds
It isn't like it runs different tests?

No idea at all as of yet... I've been trying to track that down to no avail

AlanGriffiths · 2024-06-11T10:41:05Z

OK, my problems here are not so much with the test harness itself, but with the restructuring needed to support it.

And that actually comes from the legacy structure of the project.

There's an "undefined behaviour" problem with including both miral and miral-internal, as miral include miral-internal (internally) which gives two instances of any code from miral-internal (breaks the ODR). (I am aware that, as these copies are identical and not exported, it is unlikely we're doing anything that breaks in practice.)
Publishing even more mirserver internals "just" to use them in a test harness seems wrong. We should have a better way. (I did a slightly better job with miral, as there is a separate miral-internal library that could be included in test binaries, but as evidenced by /1/ it isn't perfect.)

I need to think about what the options are and either propose a solution or reconcile myself to the "ugliness".

mattkae · 2024-06-12T11:31:59Z

Symbols issue will be fixed by #3423

mattkae · 2024-06-12T11:33:20Z

OK, my problems here are not so much with the test harness itself, but with the restructuring needed to support it.

And that actually comes from the legacy structure of the project.
1. There's an "undefined behaviour" problem with including both `miral` and `miral-internal`, as `miral` include `miral-internal` (internally) which gives two instances of any code from `miral-internal` (breaks the ODR). (I am aware that, as these copies are identical and not exported, it is unlikely we're doing anything that breaks in practice.)

2. Publishing even more mirserver internals "just" to _use_ them in a test harness seems wrong. We should have a better way. (I did a slightly better job with miral, as there is a separate `miral-internal` library that could be included in test binaries, but as evidenced by /1/ it isn't perfect.)
I need to think about what the options are and either propose a solution or reconcile myself to the "ugliness".

I have moved the tests to their own suite, so let me know what you want to do in this domain.

codecov · 2024-06-12T11:36:19Z

Codecov Report

Attention: Patch coverage is 95.18900% with 14 lines in your changes missing coverage. Please review.

Project coverage is 77.46%. Comparing base (0980da3) to head (a19d741).
Report is 412 commits behind head on main.

Files with missing lines	Patch %	Lines
..._test_framework/window_management_test_harness.cpp	82.19%	13 Missing ⚠️
...w_management_tests/test_minimal_window_manager.cpp	99.54%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3416      +/-   ##
==========================================
+ Coverage   77.24%   77.46%   +0.22%     
==========================================
  Files        1071     1073       +2     
  Lines       68467    68759     +292     
==========================================
+ Hits        52886    53263     +377     
+ Misses      15581    15496      -85

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AlanGriffiths

I'm still experimenting with ideas to clean up using our library internals for testing

tests/window_management_tests/CMakeLists.txt

tests/CMakeLists.txt

tests/integration-tests/CMakeLists.txt

tests/include/mir_test_framework/window_management_test_harness.h

RAOF

This looks like a good start (I've some code-style nits with not using our auto foo() -> bar default in places).

This is not quite what I was expecting - rather than validating that the expected WM hooks are called at the appropriate time, I was expecting that the WM state would be validated at the end. Something like

<create windows>
EXPECT_THAT(stacking_order(), Eq({window1, window2, window3, window4});
<emit alt-tab sequence>
EXPECT_THAT(stacking_order(), Eq({window2, window1, window3, window4})

and suchlike.

I think that would be both simpler to implement and would be more stable over code changes (we're not testing the implementation details of how WM happens, we're testing what window management happens).

This PR also basically implements that, right? You'd turn get_shell et al into a set of focused_surface(), stacking_order(), etc methods, as Alan suggests, and then maybe (re)move a bunch of code (like the whole WindowManagementVerifier infrastructure) into a separate set of tests?

tests/include/mir/test/doubles/stub_input_seat.h

tests/integration-tests/CMakeLists.txt

tests/window_management_tests/test_minimal_window_manager.cpp

tests/window_management_tests/CMakeLists.txt

mattkae · 2024-06-13T11:47:35Z

This looks like a good start (I've some code-style nits with not using our auto foo() -> bar default in places).

This is not quite what I was expecting - rather than validating that the expected WM hooks are called at the appropriate time, I was expecting that the WM state would be validated at the end. Something like
<create windows>
EXPECT_THAT(stacking_order(), Eq({window1, window2, window3, window4});
<emit alt-tab sequence>
EXPECT_THAT(stacking_order(), Eq({window2, window1, window3, window4})
and suchlike.

I think that would be both simpler to implement and would be more stable over code changes (we're not testing the implementation details of how WM happens, we're testing what window management happens).

This PR also basically implements that, right? You'd turn get_shell et al into a set of focused_surface(), stacking_order(), etc methods, as Alan suggests, and then maybe (re)move a bunch of code (like the whole WindowManagementVerifier infrastructure) into a separate set of tests?

Yeah I agree. The WindowManagementVerifier got a little out of hand. I am going to remove it, but the rest is still valid 😄

RAOF

Hm. I think this could be dramatically simplified by using mtf::HeadlessTest and mtf::add_fake_input_device?

What the WindowManagementTestHarness requires is the ability to inject input events (provided by mtf::FakeInputDevice) and access to the_shell() and the_surface_stack(), both of which are already provided by the mir::Server you can get from mtf::HeadlessTest. Oh, and to set the actual window manager under test, which you can do with mir::Server::override_the_window_manager_builder?

That way you wouldn't need to expose a bunch of extra stuff, or do dummy implementations of some of the stuff.

tests/include/mir_test_framework/window_management_test_harness.h

mattkae · 2024-06-14T11:19:25Z

Hm. I think this could be dramatically simplified by using mtf::HeadlessTest and mtf::add_fake_input_device?

What the WindowManagementTestHarness requires is the ability to inject input events (provided by mtf::FakeInputDevice) and access to the_shell() and the_surface_stack(), both of which are already provided by the mir::Server you can get from mtf::HeadlessTest. Oh, and to set the actual window manager under test, which you can do with mir::Server::override_the_window_manager_builder?

That way you wouldn't need to expose a bunch of extra stuff, or do dummy implementations of some of the stuff.

Hm, that is a really good point! We could indeed just wrap HeadlessTest and have nearly the same functionality (albeit a bit heaver, bordering on end-to-end test). I can't think of any push back off of the top of my head, but maybe @AlanGriffiths can.

AlanGriffiths · 2024-06-14T11:31:38Z

Hm, that is a really good point! We could indeed just wrap HeadlessTest and have nearly the same functionality (albeit a bit heaver, bordering on end-to-end test). I can't think of any push back off of the top of my head, but maybe @AlanGriffiths can.

It is a good point: I hadn't recognised the commonality

mattkae · 2024-06-18T12:32:49Z

@RAOF and @AlanGriffiths : Yup, using the HeadlessInProcessServer was indeed the answer. We still need a SurfaceObserver to get the timings right (usually this would be a Wayland-specific SurfaceObserver), but this has simplified everything else pretty dramatically.

RAOF

This is a great simplification. I think this can be simplified even further (and could be slightly nicer stylistically by using the ms:: etc namespace aliases consistently throughout).

The only blocking thing here is the comment in ::SetUp().

tests/mir_test_framework/window_management_test_harness.cpp

RAOF · 2024-06-20T00:45:17Z

tests/mir_test_framework/window_management_test_harness.cpp

+    mir_test_framework::HeadlessInProcessServer::SetUp();
+    self = std::make_shared<Self>(this);
+
+    auto fake_display = std::make_unique<mtd::FakeDisplay>(get_output_rectangles());
+    self->display = fake_display.get();
+    preset_display(std::move(fake_display));


Hm. Doesn't preset_display need to be called before HeadlessInProcessServer::SetUp()? ::SetUp() will start the server, which will include setting up the display?

tests/miral/active_outputs.cpp has the same order for some reason? I was following the example, but I would imagine that you are correct!

tests/mir_test_framework/window_management_test_harness.cpp

tests/include/mir_test_framework/window_management_test_harness.h

RAOF

You appear to have accidentally a test 😉

The workqueue still appears weird to me; the only use is to have client_surface_close_requested call destroy_surface in a deferred fashion? So the only time it's actually interesting is for publish_event? And that matters because the WM takes a lock when processing the input event, and shell->destroy_surface would also try to take that lock.

Right?

RAOF · 2024-06-26T07:20:25Z

tests/window_management_tests/test_minimal_window_manager.cpp

+TEST_F(MinimalWindowManagerTest, alt_f4_closes_active_window)
+{
+    auto const app = open_application("test");
+    msh::SurfaceSpecification spec;
+    spec.width = geom::Width {100};
+    spec.height = geom::Height{100};
+    spec.depth_layer = mir_depth_layer_application;
+    auto window = create_window(app, spec);
+
+    // Process an alt + f4 input
+    std::chrono::nanoseconds const event_timestamp = std::chrono::system_clock::now().time_since_epoch();
+    MirKeyboardAction const action{mir_keyboard_action_down};
+    xkb_keysym_t const keysym{0};
+    int const scan_code{KEY_F4};
+    MirInputEventModifiers const modifiers{mir_input_event_modifier_alt};
+    auto const event = mir::events::make_key_event(
+        mir_input_event_type_key,
+        event_timestamp,
+        action,
+        keysym,
+        scan_code,
+        modifiers);
+    publish_event(*event);
+}


Where is the check that the window has been closed?

RAOF · 2024-06-26T07:39:09Z

tests/mir_test_framework/window_management_test_harness.cpp

+        auto const& f = work_queue.front();
+        f();
+        work_queue.pop();


This can result in infinite recursion, right? Almost all usages of the workqueue are of the form queue.enqueue(...); queue.run(), and if that gets run from the workqueue it'll push something to the back of the queue and then run itself again?

Yeah I agree that it's kind of nasty... I am trying to find a good way to avoid both the nastiness of the queue and the nastiness of asynchronicity

After thinking about this deeper, it can result in recursion, but this isn't necessarily a bad thing. The WorkQueue will just continually be run recursively, which will work.

If you end up in a situation where the recursion is infinite, then that suggests that your WM is doing something wrong. e.g. if every time you resize a window, you also move it, and every time you move a window, you also resize it, then that is on you

So, the problem I'm talking about is running f() before running work_queue.pop(). If f() happens to run work_queue.run(), then work_queue.run() will call work_queue.front(), receive the same f again, and recurse.

If you popped before calling f() (and std::move()d out of front()), that wouldn't happen.

mattkae · 2024-06-26T11:15:22Z

You appear to have accidentally a test 😉

The workqueue still appears weird to me; the only use is to have client_surface_close_requested call destroy_surface in a deferred fashion? So the only time it's actually interesting is for publish_event? And that matters because the WM takes a lock when processing the input event, and shell->destroy_surface would also try to take that lock.

Right?

Yes exactly. (Although... I wish it wasn't the case. Let me see if there's any way around it one last time)

mattkae · 2024-06-26T13:56:29Z

@RAOF: I see no clever way around the WorkQueue at the moment. This queue at least avoids some complexities with threading, which I'm a fan of. I think that users can get themselves in sticky situations only when their WM is doing something wrong.

tests/mir_test_framework/window_management_test_harness.cpp

RAOF

So, the WorkQueue seems like it could change from a work queue into just a std::vector<std::shared_ptr<ms::Surface>> pending_surfaces_to_destroy; plus a loop at the end of publish_event destroying those surfaces.

Neither request_resize nor request_move actually use the work_queue, right? They push a single functor into the queue and then immediately pop that functor off the front and run it - there's no way for client_surface_close_requested to be called from either of those codepaths?

I won't block on this, but I feel like that would be simpler?

RAOF · 2024-06-27T02:29:38Z

tests/mir_test_framework/window_management_test_harness.cpp

+        auto const& f = work_queue.front();
+        f();
+        work_queue.pop();


So, the problem I'm talking about is running f() before running work_queue.pop(). If f() happens to run work_queue.run(), then work_queue.run() will call work_queue.front(), receive the same f again, and recurse.

If you popped before calling f() (and std::move()d out of front()), that wouldn't happen.

mattkae · 2024-06-27T11:44:54Z

So, the WorkQueue seems like it could change from a work queue into just a std::vector<std::shared_ptr<ms::Surface>> pending_surfaces_to_destroy; plus a loop at the end of publish_event destroying those surfaces.

Neither request_resize nor request_move actually use the work_queue, right? They push a single functor into the queue and then immediately pop that functor off the front and run it - there's no way for client_surface_close_requested to be called from either of those codepaths?

I won't block on this, but I feel like that would be simpler?

I can agree with that 👍 Really only the destroy code needs it for now

mattkae · 2024-06-27T11:46:18Z

@RAOF I also better understand the issue now, misunderstood before. I will pop before calling f

Edit: Maybe you're all-around right though, and the WorkQueue is overkill

mattkae · 2024-06-27T12:03:32Z

@RAOF: Now that I fully grasp your point, you were right all along 😄

AlanGriffiths

One nit, but otherwise good to go

AlanGriffiths · 2024-06-25T09:25:43Z

tests/mir_test_framework/window_management_test_harness.cpp

+
+void mir_test_framework::WindowManagementTestHarness::SetUp()
+{
+    self = std::make_shared<Self>(this);


Surely this initialization can happen in the constructor?

AlanGriffiths · 2024-06-28T13:31:54Z

tests/mir_test_framework/CMakeLists.txt

+  ${PROJECT_SOURCE_DIR}/include/miral
+  ${PROJECT_SOURCE_DIR}/src/miral


${PROJECT_SOURCE_DIR}/src/miral is not needed

…ndowManager

mattkae changed the title ~~(DO NOT REVIEW)~~ feature: a test harness for window managers + tests for the MinimalWindowManager Jun 7, 2024

mattkae force-pushed the feature/window_management_testing branch 3 times, most recently from 96ab7f1 to 813e812 Compare June 7, 2024 18:43

mattkae requested a review from AlanGriffiths June 7, 2024 19:47

mattkae marked this pull request as ready for review June 7, 2024 19:47

mattkae requested a review from a team as a code owner June 7, 2024 19:47

AlanGriffiths reviewed Jun 10, 2024

View reviewed changes

src/server/symbols.map Outdated Show resolved Hide resolved

AlanGriffiths reviewed Jun 10, 2024

View reviewed changes

mattkae requested a review from AlanGriffiths June 10, 2024 19:05

mattkae force-pushed the feature/window_management_testing branch 2 times, most recently from b2d4d96 to 3427ed9 Compare June 11, 2024 19:22

AlanGriffiths requested changes Jun 12, 2024

View reviewed changes

RAOF reviewed Jun 13, 2024

View reviewed changes

tests/window_management_tests/test_minimal_window_manager.cpp Outdated Show resolved Hide resolved

AlanGriffiths reviewed Jun 13, 2024

View reviewed changes

tests/window_management_tests/CMakeLists.txt Outdated Show resolved Hide resolved

tests/window_management_tests/CMakeLists.txt Outdated Show resolved Hide resolved

tests/window_management_tests/CMakeLists.txt Outdated Show resolved Hide resolved

mattkae requested review from RAOF and AlanGriffiths June 13, 2024 13:13

RAOF reviewed Jun 14, 2024

View reviewed changes

tests/include/mir_test_framework/window_management_test_harness.h Outdated Show resolved Hide resolved

mattkae requested a review from RAOF June 18, 2024 12:32

RAOF requested changes Jun 20, 2024

View reviewed changes

mattkae requested a review from RAOF June 20, 2024 18:15

mattkae force-pushed the feature/window_management_testing branch from b8b0cdb to d7787a1 Compare June 20, 2024 19:14

AlanGriffiths reviewed Jun 24, 2024

View reviewed changes

tests/include/mir_test_framework/window_management_test_harness.h Outdated Show resolved Hide resolved

mattkae force-pushed the feature/window_management_testing branch from 9e92db1 to fc10eda Compare June 24, 2024 13:15

mattkae requested a review from AlanGriffiths June 24, 2024 13:58

RAOF requested changes Jun 26, 2024

View reviewed changes

mattkae requested a review from RAOF June 26, 2024 13:56

RAOF reviewed Jun 27, 2024

View reviewed changes

tests/mir_test_framework/window_management_test_harness.cpp Outdated Show resolved Hide resolved

RAOF reviewed Jun 27, 2024

View reviewed changes

tests/mir_test_framework/window_management_test_harness.cpp Outdated Show resolved Hide resolved

RAOF approved these changes Jun 27, 2024

View reviewed changes

mattkae force-pushed the feature/window_management_testing branch from 4273f01 to 3a26b1e Compare June 27, 2024 12:05

AlanGriffiths approved these changes Jun 28, 2024

View reviewed changes

feature: a test harness for window managers + tests for the MinimalWi…

a19d741

…ndowManager

mattkae force-pushed the feature/window_management_testing branch from 3a26b1e to a19d741 Compare June 28, 2024 13:47

mattkae added this pull request to the merge queue Jun 28, 2024

Merged via the queue into main with commit f267755 Jun 28, 2024
24 of 25 checks passed

mattkae deleted the feature/window_management_testing branch June 28, 2024 16:03

		${PROJECT_SOURCE_DIR}/include/miral
		${PROJECT_SOURCE_DIR}/src/miral

feature: a test harness for window managers + tests for the MinimalWindowManager #3416

feature: a test harness for window managers + tests for the MinimalWindowManager #3416

Conversation

mattkae commented Jun 6, 2024 • edited Loading

What's new?

AlanGriffiths commented Jun 10, 2024

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkae commented Jun 10, 2024

AlanGriffiths commented Jun 11, 2024

mattkae commented Jun 12, 2024

mattkae commented Jun 12, 2024

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

AlanGriffiths left a comment

Choose a reason for hiding this comment

RAOF left a comment

Choose a reason for hiding this comment

mattkae commented Jun 13, 2024

RAOF left a comment • edited Loading

Choose a reason for hiding this comment

mattkae commented Jun 14, 2024

AlanGriffiths commented Jun 14, 2024

mattkae commented Jun 18, 2024

RAOF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RAOF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkae commented Jun 26, 2024 • edited Loading

mattkae commented Jun 26, 2024

RAOF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkae commented Jun 27, 2024

mattkae commented Jun 27, 2024 • edited Loading

mattkae commented Jun 27, 2024

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkae commented Jun 6, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading

RAOF left a comment •

edited

Loading

mattkae commented Jun 26, 2024 •

edited

Loading

mattkae commented Jun 27, 2024 •

edited

Loading