Add more comprehensive wl_output tests by robert-ancell · Pull Request #445 · canonical/wlcs

robert-ancell · 2026-06-16T01:57:31Z

Replace the two existing wl_output tests with a more thorough set that verifies each property advertised in the initial output advertisement.

A mockable WlOutputListener wraps the wl_output events so the tests can assert the advertised geometry, current mode, scale and name via EXPECT_CALL, and confirm the advertisement is terminated by exactly one done event.

Replace the two existing wl_output tests with a more thorough set that verifies each property advertised in the initial output advertisement. A mockable WlOutputListener wraps the wl_output events so the tests can assert the advertised geometry, current mode, scale and name via EXPECT_CALL, and confirm the advertisement is terminated by exactly one done event. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR expands wl_output protocol coverage by replacing broad state-based assertions with event-level expectations, allowing tests to validate each advertised property and the done termination event.

Changes:

Introduces a mockable WlOutputListener wrapper that forwards wl_output events into virtual methods.
Adds targeted tests for geometry, current mode, scale, name, and “exactly one done” after initial advertisement.
Refactors the test fixture to include a helper that dispatches until the initial wl_output.done is observed.

jhenstridge · 2026-06-16T07:54:23Z

+        EXPECT_CALL(listener, done(_)).WillRepeatedly(Invoke([&done](auto) { done = true; }));
+        client.dispatch_until([&done]() { return done; });


This WillRepeatedly lambda may continue to be invoked after the method returns. The reference to the done local will be out of scope at that point, so you're overwriting whatever happens to take its place on the stack.

You probably want a Mock::VerifyAndClearExpectations call to clean things up, and perhaps install a new handler if you want to handle further done events. Check create_xdg_shell_stable_surface for a similar pattern.

I changed it to ON_CALL/WillByDefault as suggested by Gemini. This is used elsewhere. Do you think this is appropriate?

I don't think WillByDefault makes things any better, since the callback will still be attached to the mock after the captured reference is out of scope.

Maybe this would be the most concise and correct version?

EXPECT_CALL(listener, done()).WillOnce(Invoke([&done](auto) { done = true; })).RetiresOnSaturation();

That should remove the expectation from the mock after the event is dispatched, so it doesn't last past the end of the function call.

The done listener method is only stubbed to drive the dispatch loop, not asserted, so ON_CALL/WillByDefault expresses the intent better than an EXPECT_CALL with an implicit any-cardinality WillRepeatedly.

Replace the abstract base class plus separate mock subclass and the hand-written static thunks with a single mock class that uses MOCK_METHOD directly and forwards events via inline FORWARD_TO_MOCK lambdas, matching the listener pattern used by the other tests. The redundant wl_output* argument is dropped from each event since a listener is bound to a single output.

jhenstridge

There's a few things that need fixing here, but it mostly comes down to two main issues:

All the tests that rely on the done event require at least version 2 of the interface.
The use of NiceMock means unexpected calls will be ignored. It might be worth adding a catch all EXPECT_CALL(listener, method(_)).Times(0); expectation to explicitly fail on events with bad arguments.

jhenstridge · 2026-07-06T02:34:10Z

+    {
+        bool done = false;
+        ON_CALL(listener, done()).WillByDefault(Invoke([&done]() { done = true; }));
+        client.dispatch_until([&done]() { return done; });


There should probably be a VerifyAndClearExpectations call here to ensure the closure is not called again after the done local is out of scope.

I realise that you're only calling this method at the end of test methods, but it would make it more difficult to misuse.

jhenstridge · 2026-07-06T03:47:52Z

+    auto const output = client.bind_if_supported<wl_output>(wlcs::AnyVersion);
+    NiceMock<WlOutputListener> listener{output};
+
+    EXPECT_CALL(listener, scale(Ge(1)));


The protocol documentation says this event is optional, so this doesn't seem quite right.

Adding .Times(AtMost(1)) to this expectation would handle the case of the event not being sent. But because you're using NiceMock, any event received with factor < 1 is going to be ignored due to not matching any expectations. So maybe the following would be better?

Suggested change

EXPECT_CALL(listener, scale(Ge(1)));

EXPECT_CALL(listener, scale(_)).Times(0);

EXPECT_CALL(listener, scale(Ge(1))).Times(AtMost(1));

The first expectation will fail on any bad scale events, and the second will accept any good scale events.

It might be worth doing the same for the other tests: while they will fail if an event with the expected arguments doesn't arrive, they won't tell you about unexpected events.

jhenstridge · 2026-07-06T04:27:02Z

+{
+    wlcs::Client client{the_server()};
+
+    auto const output = client.bind_if_supported<wl_output>(wlcs::AnyVersion);


The scale event was added in version 2 of the interface.

Suggested change

auto const output = client.bind_if_supported<wl_output>(wlcs::AnyVersion);

auto const output = client.bind_if_supported<wl_output>(wlcs::AtLeastVersion{WL_OUTPUT_SCALE_SINCE_VERSION});

jhenstridge · 2026-07-06T04:39:44Z

+{
+    wlcs::Client client{the_server()};
+
+    auto const output = client.bind_if_supported<wl_output>(wlcs::AnyVersion);


The done event was added in version 2 of the interface.

Suggested change

auto const output = client.bind_if_supported<wl_output>(wlcs::AnyVersion);

auto const output = client.bind_if_supported<wl_output>(wlcs::AtLeastVersion{WL_OUTPUT_DONE_SINCE_VERSION});

In fact, any tests using the receive_initial_properties helper will fail without that version, so I don't think AnyVersion is appropriate for any of the tests here.

jhenstridge · 2026-07-06T04:48:27Z

+    auto const output = client.bind_if_supported<wl_output>(wlcs::AtLeastVersion{WL_OUTPUT_NAME_SINCE_VERSION});
+    NiceMock<WlOutputListener> listener{output};
+
+    EXPECT_CALL(listener, name(StrNe("")));


It might also be useful to check that no two outputs return the same name, but I'm not sure how you'd bind the other outputs via bind_if_supported.

Copilot AI review requested due to automatic review settings June 16, 2026 01:57

robert-ancell requested a review from a team as a code owner June 16, 2026 01:57

Copilot started reviewing on behalf of robert-ancell June 16, 2026 02:03 View session

Copilot AI reviewed Jun 16, 2026

View reviewed changes

Comment thread tests/wl_output.cpp Outdated

Comment thread tests/wl_output.cpp Outdated

Comment thread tests/wl_output.cpp

jhenstridge reviewed Jun 16, 2026

View reviewed changes

RAOF reviewed Jun 16, 2026

View reviewed changes

Comment thread tests/wl_output.cpp Outdated

robert-ancell added 3 commits June 24, 2026 16:04

Merge branch 'main' into more-comprehensive-wl-output-tests

131e261

Use ON_CALL for the done stub in receive_initial_properties

dd2d88d

The done listener method is only stubbed to drive the dispatch loop, not asserted, so ON_CALL/WillByDefault expresses the intent better than an EXPECT_CALL with an implicit any-cardinality WillRepeatedly.

robert-ancell requested review from RAOF and jhenstridge June 24, 2026 04:16

jhenstridge requested changes Jul 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

Add more comprehensive wl_output tests - #445

Add more comprehensive wl_output tests#445
robert-ancell wants to merge 4 commits into
mainfrom
more-comprehensive-wl-output-tests

robert-ancell commented Jun 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhenstridge Jun 16, 2026

Uh oh!

robert-ancell Jun 24, 2026

Uh oh!

jhenstridge Jul 6, 2026

Uh oh!

Uh oh!

jhenstridge left a comment

Uh oh!

jhenstridge Jul 6, 2026

Uh oh!

jhenstridge Jul 6, 2026

Uh oh!

jhenstridge Jul 6, 2026

Uh oh!

jhenstridge Jul 6, 2026

Uh oh!

jhenstridge Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		EXPECT_CALL(listener, done(_)).WillRepeatedly(Invoke([&done](auto) { done = true; }));
		client.dispatch_until([&done]() { return done; });

	EXPECT_CALL(listener, scale(Ge(1)));
	EXPECT_CALL(listener, scale(_)).Times(0);
	EXPECT_CALL(listener, scale(Ge(1))).Times(AtMost(1));

	auto const output = client.bind_if_supported<wl_output>(wlcs::AnyVersion);
	auto const output = client.bind_if_supported<wl_output>(wlcs::AtLeastVersion{WL_OUTPUT_SCALE_SINCE_VERSION});

Uh oh!

Conversation

robert-ancell commented Jun 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhenstridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants