Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 3, 2025

WindowManagementTestHarness provided no access to MirRunner functionality (start/stop callbacks, signal handlers, fd handlers) that window management policies depend on in production, creating a test/prod environment gap.

Changes

  • Header: Added five methods matching MirRunner's API: add_start_callback(), add_stop_callback(), register_signal_handler(), register_fd_handler(), invoke_runner()

  • Implementation: Uses actual miral::MirRunner instance with pure delegation:

    • All methods delegate directly to MirRunner without any wrapper logic
    • No local callback storage or execution - relies entirely on MirRunner's internal state
    • Callbacks registered via these methods are stored in MirRunner but won't execute since WindowManagementTestHarness doesn't use MirRunner::run_with()
  • Tests: Added test_mir_runner_access.cpp validating API availability without expecting callback execution

Usage

class MyWindowManagerTest : public WindowManagementTestHarness
{
public:
    MyWindowManagerTest()
    {
        add_start_callback([this] {
            // Callback stored in MirRunner (won't execute in current implementation)
        });
        
        register_signal_handler({SIGUSR1}, [](int) {
            // Handler stored in MirRunner (won't execute in current implementation)
        });
        
        auto fd_handle = register_fd_handler(my_fd, [](int) {
            // Handler registered via MirRunner's FdManager
        });
    }
};

API consistent with TestDisplayServer::invoke_runner().

Note: The API provides compatibility for code that expects MirRunner functionality, but callbacks won't execute unless the harness is refactored to use MirRunner::run_with() for server lifecycle management.

Original prompt

This section details on the original issue you should resolve

<issue_title>Add MirRunner access to WindowManagementTestHarness - Implementation Plan</issue_title>
<issue_description># Add MirRunner access to WindowManagementTestHarness - Implementation Plan

Background

As identified in issue #4107, the WindowManagementTestHarness currently provides no access to a miral::MirRunner, which many window management policies rely on for operations like:

  • Scheduling tasks via add_start_callback() and add_stop_callback()
  • Registering file descriptor handlers with register_fd_handler()
  • Signal handling with register_signal_handler()

This creates a gap between test environments and real-world usage, making it difficult to properly test policies that depend on MirRunner functionality.

Implementation Locations

1. Header File Changes

File: /include/test/mir_test_framework/window_management_test_harness.h

Add the following public methods to the WindowManagementTestHarness class:

/// Add a callback to be invoked when the server has started
void add_start_callback(std::function<void()> const& start_callback);

/// Add a callback to be invoked when the server is about to stop  
void add_stop_callback(std::function<void()> const& stop_callback);

/// Add signal handler to the server's main loop
void register_signal_handler(std::initializer_list<int> signals, std::function<void(int)> const& handler);

/// Add a watch on a file descriptor to the server's main loop
auto register_fd_handler(mir::Fd fd, std::function<void(int)> const& handler) -> std::unique_ptr<miral::FdHandle>;

/// Wrapper to gain access to the MirRunner
void invoke_runner(std::function<void(miral::MirRunner& runner)> const& f);

2. Implementation File Changes

File: /tests/mir_test_framework/window_management_test_harness.cpp

A. Update Self class (around line 310)

Add MirRunner instance to the Self class:

class mir_test_framework::WindowManagementTestHarness::Self
{
public:
    explicit Self(mir::Server& server)
        : server(server),
          runner(std::make_unique<miral::MirRunner>(/* args needed */)),
          surface_observer(...)
    {
    }
    
    // ... existing members ...
    std::unique_ptr<miral::MirRunner> runner;
};

B. Update SetUp method (around line 412)

Integrate MirRunner lifecycle with test harness:

void mir_test_framework::WindowManagementTestHarness::SetUp()
{
    // Configure MirRunner before calling parent SetUp
    // Need to coordinate with existing setup...
    
    HeadlessInProcessServer::SetUp();
}

C. Add method implementations (after line 585)

Implement all the new MirRunner access methods by delegating to self->runner.

Implementation Challenges

  1. Lifecycle Coordination: The MirRunner needs to be properly integrated with the existing server startup/shutdown sequence
  2. Arguments: MirRunner constructor requires argc/argv which aren't readily available in the test context
  3. Server Integration: Need to ensure the MirRunner's mir::Server instance is the same one used by the test harness

Alternative Approach

Consider making WindowManagementTestHarness inherit from or compose with TestDisplayServer instead of HeadlessInProcessServer. TestDisplayServer already has full MirRunner integration via its invoke_runner() method.

Pros:

  • Reuses existing, proven MirRunner integration
  • Simpler implementation
  • Consistent with other test infrastructure

Cons:

  • Larger change to inheritance hierarchy
  • May require updating existing test code

Acceptance Criteria

  • WindowManagementTestHarness provides access to MirRunner functionality
  • Existing tests continue to pass
  • New tests can use MirRunner features (callbacks, fd handlers, signals)
  • Test policies that depend on MirRunner work correctly
  • API is consistent with TestDisplayServer::invoke_runner()

Related Issues

Implementation Notes

This change will enable more realistic testing of window management policies that use MirRunner features, closing the gap between test and production environments.</issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits November 3, 2025 11:05
Co-authored-by: mattkae <25062299+mattkae@users.noreply.github.com>
Co-authored-by: mattkae <25062299+mattkae@users.noreply.github.com>
@mattkae
Copy link
Contributor

mattkae commented Nov 3, 2025

@copilot Can you make it so that we only use MirRunner, and that we no longer rely on the wrapper methods. Is that possible?

Co-authored-by: mattkae <25062299+mattkae@users.noreply.github.com>
Copilot AI changed the title [WIP] Add MirRunner access to WindowManagementTestHarness - Implementation Plan Add MirRunner access to WindowManagementTestHarness Nov 3, 2025
Copilot AI requested a review from mattkae November 3, 2025 11:19
Co-authored-by: mattkae <25062299+mattkae@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 3, 2025

@copilot Can you make it so that we only use MirRunner, and that we no longer rely on the wrapper methods. Is that possible?

Done in f45478a. Removed all wrapper callback storage and manual execution logic. All methods now purely delegate to the MirRunner instance without any intermediate handling.

@Saviq Saviq force-pushed the main branch 4 times, most recently from 39740d9 to 43ac49a Compare December 15, 2025 17:06
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.

Add MirRunner access to WindowManagementTestHarness - Implementation Plan

2 participants