Skip to content

add workspace manager unit tests#32861

Open
luapmartin wants to merge 1 commit intomusescore:masterfrom
luapmartin:luapmartin/10510
Open

add workspace manager unit tests#32861
luapmartin wants to merge 1 commit intomusescore:masterfrom
luapmartin:luapmartin/10510

Conversation

@luapmartin
Copy link
Copy Markdown
Contributor

@luapmartin luapmartin commented Mar 30, 2026

Resolves: add workspace manager unit tests

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

Summary by CodeRabbit

  • Tests
    • Added comprehensive workspace test suite covering initialization, fallback, and data-loading scenarios
    • Introduced mock implementations for multi-window and workspace configuration to enable isolated testing
    • Added test environment setup and build/test integration to streamline running and maintaining workspace tests

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

📝 Walkthrough

Walkthrough

Adds a workspace test suite: two GMock-based mocks, test environment setup, three WorkspaceManager tests, and CMake entries to build the test module.

Changes

Cohort / File(s) Summary
MultiWindows mock
src/framework/multiwindows/tests/mocks/multiwindowsprovidermock.h
New GMock-based muse::mi::MultiWindowsProviderMock implementing the full IMultiWindowsProvider surface (window lifecycle, prefs, settings transactions, resource lock/notify, quit flows).
Workspace configuration mock
src/framework/workspace/tests/mocks/workspaceconfigurationmock.h
New GMock-based muse::workspace::WorkspaceConfigurationMock implementing IWorkspaceConfiguration (workspace paths, user/builtin paths, names, setter, async name-changed channel).
CMake test integration
src/framework/workspace/CMakeLists.txt, src/framework/workspace/tests/CMakeLists.txt
workspace CMake now conditionally includes tests when MUSE_MODULE_WORKSPACE_TESTS is set; added muse_workspace_tests target with sources, includes, link to muse::workspace, and BUILTIN_WORKSPACES_DIR definition.
Test environment
src/framework/workspace/tests/environment.cpp
New test SuiteEnvironment (workspace_se) that registers a NiceMock<WorkspaceConfigurationMock> in the global IoC under "utests" during pre-init and removes it on de-init; stubs defaultWorkspaceName() to "Default".
WorkspaceManager tests
src/framework/workspace/tests/workspacemanager_tests.cpp
Adds test fixture and three GoogleTest cases verifying builtin workspace load on init, fallback to default when current is missing (expects setCurrentWorkspaceName("Default")), and behavior with empty user dir (loads builtin only and verifies ui_settings rawData).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title accurately describes the main change: adding unit tests for the workspace manager, which aligns with all the test files and infrastructure added in the changeset.
Description check ✅ Passed The PR description follows the template structure and includes all required sections with checkboxes completed, though the 'Resolves' line references the PR objective instead of a specific GitHub issue.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a3397d9a-88a8-4211-8081-9cc7f9022c16

📥 Commits

Reviewing files that changed from the base of the PR and between 18e7b54 and c10aa51.

📒 Files selected for processing (6)
  • src/framework/multiwindows/tests/mocks/multiwindowsprovidermock.h
  • src/framework/workspace/CMakeLists.txt
  • src/framework/workspace/tests/CMakeLists.txt
  • src/framework/workspace/tests/environment.cpp
  • src/framework/workspace/tests/mocks/workspaceconfigurationmock.h
  • src/framework/workspace/tests/workspacemanager_tests.cpp

Comment on lines +57 to +59
if (MUSE_MODULE_WORKSPACE_TESTS)
add_subdirectory(tests)
endif()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify declaration and usage of the workspace tests toggle.
rg -n --type=cmake -C2 '\bMUSE_MODULE_WORKSPACE_TESTS\b'

Repository: musescore/MuseScore

Length of output: 350


Add or declare the MUSE_MODULE_WORKSPACE_TESTS toggle before use.

The variable MUSE_MODULE_WORKSPACE_TESTS is checked at line 57 but is never declared or set anywhere in the codebase. In CMake, undefined variables evaluate to false in conditionals, which means the tests directory will never be included regardless of intent. Either remove this check if tests are always needed, or declare the option/variable (e.g., option(MUSE_MODULE_WORKSPACE_TESTS "Enable workspace tests" OFF)) in the parent CMakeLists.txt or cmake configuration.

Comment on lines +59 to +63
void TearDown() override
{
m_manager.reset();
modularity::globalIoc()->unregister<mi::IMultiWindowsProvider>("utests");
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Move manager deinitialization into TearDown() for guaranteed cleanup.

Right now cleanup depends on reaching the end of each test body. A fatal assertion can skip deinit() and leave state behind for subsequent tests.

Proposed fix
 void TearDown() override
 {
-    m_manager.reset();
+    if (m_manager) {
+        m_manager->deinit();
+        m_manager.reset();
+    }
     modularity::globalIoc()->unregister<mi::IMultiWindowsProvider>("utests");
 }
@@
-    m_manager->deinit();
 }
@@
-    m_manager->deinit();
 }
@@
-    m_manager->deinit();
 }

Also applies to: 118-119, 143-144, 169-170

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.

2 participants