From c1d42bdadd329c5ab175cdb96cf5c74a179fa13f Mon Sep 17 00:00:00 2001 From: Sebastian Dine Date: Sat, 9 May 2026 19:57:02 +0200 Subject: [PATCH] patch: sonarqube fixes. --- .github/workflows/AGENTS.md | 1 + .github/workflows/feature-ci.yml | 1 + .github/workflows/master-ci.yml | 1 + AGENTS.md | 4 +- docs/ci-cd-guide.md | 4 ++ docs/testing-and-test-code-of-conduct.md | 2 + tests/AGENTS.md | 1 + tests/CMakeLists.txt | 1 + tests/local_image_store_tests.cpp | 90 ++++++++++++++++++++++++ ui_wx/AGENTS.md | 2 +- ui_wx/include/ccm/ui/Theme.hpp | 2 + ui_wx/src/MagicGameView.cpp | 17 +---- ui_wx/src/PokemonGameView.cpp | 17 +---- ui_wx/src/Theme.cpp | 8 +++ ui_wx/src/YuGiOhGameView.cpp | 17 +---- 15 files changed, 123 insertions(+), 45 deletions(-) create mode 100644 tests/local_image_store_tests.cpp diff --git a/.github/workflows/AGENTS.md b/.github/workflows/AGENTS.md index b424f40..ee5a152 100644 --- a/.github/workflows/AGENTS.md +++ b/.github/workflows/AGENTS.md @@ -38,6 +38,7 @@ GitHub Actions workflows for CI, release automation, and policy checks. - Prefer minimal, surgical edits; avoid large workflow rewrites unless requested. - Reusable workflows should declare explicit `workflow_call` inputs for required context (e.g., version, merge SHA). - Sonar coverage steps that use `gcovr` must exclude third-party build trees at discovery time with `--exclude-directories` (for example `build/_deps`) so gcov does not process dependency `.gcda` files. +- The Sonar scan step passes `-Dsonar.coverage.exclusions=**/ui_wx/**,**/app/**` so the coverage quality gate reflects **`ccm_core_tests`** only (wx UI and the composition root are not executed under test). `sonar.sources` stays `core,ui_wx,app`; bugs/security/duplications still analyze those trees. - For Linux Sonar coverage jobs, keep compiler and gcov toolchain aligned; because `cmake/Toolchain.cmake` prefers Clang by default, set `-DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++` explicitly in the coverage configure step when using gcovr default `gcov`. - Keep `permissions` least-privilege: - reusable build workflows: `contents: read` diff --git a/.github/workflows/feature-ci.yml b/.github/workflows/feature-ci.yml index f5fa25a..4de7a75 100644 --- a/.github/workflows/feature-ci.yml +++ b/.github/workflows/feature-ci.yml @@ -69,6 +69,7 @@ jobs: -Dsonar.sources=core,ui_wx,app -Dsonar.cfamily.compile-commands=build/compile_commands.json -Dsonar.coverageReportPaths=build/sonarqube-coverage.xml + -Dsonar.coverage.exclusions=**/ui_wx/**,**/app/** linux: name: Linux build + tests diff --git a/.github/workflows/master-ci.yml b/.github/workflows/master-ci.yml index e5b18b1..3500c6c 100644 --- a/.github/workflows/master-ci.yml +++ b/.github/workflows/master-ci.yml @@ -69,6 +69,7 @@ jobs: -Dsonar.sources=core,ui_wx,app -Dsonar.cfamily.compile-commands=build/compile_commands.json -Dsonar.coverageReportPaths=build/sonarqube-coverage.xml + -Dsonar.coverage.exclusions=**/ui_wx/**,**/app/** compute-version: name: Determine semantic version diff --git a/AGENTS.md b/AGENTS.md index f63a622..a777a5e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -52,7 +52,7 @@ Run from the **workspace root**. - Run the app: `./build/bin/ccm3` (`.\build\bin\ccm3.exe` on Windows) - Run tests (CCM_BUILD_TESTS defaults to ON): - `ctest --test-dir build --output-on-failure` — current baseline: **180 tests, all green**. + `ctest --test-dir build --output-on-failure` — current baseline: **185 tests, all green**. - Build tests only: `cmake --build build --target ccm_core_tests` @@ -96,7 +96,7 @@ Run from the **workspace root**. - After modifying a domain type's fields or JSON layout you **must** update the matching round-trip test in `tests/domain_json_tests.cpp` and re-run tests. - After adding a new `.cpp` to `core/` or `ui_wx/` you **must** add it to that package's `CMakeLists.txt`. There is no glob. - After adding a new dependency you **must** verify its license is compatible with this repository's MIT license before merging. -- After changing SonarQube coverage generation, keep dependency build outputs excluded at gcov discovery time (for example `gcovr --exclude-directories "build/_deps"`); output-only excludes are not enough for third-party `.gcda` files. +- After changing SonarQube coverage generation, keep dependency build outputs excluded at gcov discovery time (for example `gcovr --exclude-directories "build/_deps"`); output-only excludes are not enough for third-party `.gcda` files. The Sonar scan uses `sonar.coverage.exclusions` for `**/ui_wx/**` and `**/app/**` so the coverage percentage matches the hermetic `ccm_core_tests` surface (`core/`); analyzed sources are unchanged for other Sonar metrics. - After adding a new game module you **must**: (1) extend `Game` enum + string mappings in `core/include/ccm/domain/Enums.hpp`, (2) register the module in `app/main.cpp`, (3) add a directory mapping in `app/main.cpp::dirNameForGame`, (4) implement an `IGameView` derived class (or `GameView`) and add it to `AppContext::gameViews` in the composition root. - After changing the per-game seams (`IGameModule`, `IGameView`, the `BaseCard*Panel` template hooks) you **must** update `docs/adding-a-new-game.md` so the canonical "add a new game" walkthrough stays in sync with the code. - After changing `formatTextForFs` or `parseIndexFromFilename` you **must** update `tests/fs_names_tests.cpp` — these functions exist to stay byte-compatible with the original Rust `util/fs.rs`. diff --git a/docs/ci-cd-guide.md b/docs/ci-cd-guide.md index 57ecc82..695babf 100644 --- a/docs/ci-cd-guide.md +++ b/docs/ci-cd-guide.md @@ -17,6 +17,10 @@ The repository uses GitHub Actions workflows split by branch intent, with one or - `master-ci.yml`: single workflow run on merged PRs to `master`; computes semver, invokes Windows reusable build, then tags/publishes release assets. - `master-windows.yml`: reusable Windows build/test/package workflow invoked by `master-ci.yml`. +### SonarQube Cloud (coverage quality gate) + +Both `feature-ci.yml` and `master-ci.yml` include a Linux job that configures with GCC coverage flags, builds, runs `ctest`, generates `build/sonarqube-coverage.xml` via `gcovr`, and runs the SonarCloud scan. **`sonar.coverage.exclusions`** omit `ui_wx/` and `app/` from the coverage denominator because only `ccm_core` is exercised by automated tests; Sonar still analyzes those directories for bugs, vulnerabilities, and duplications. See [Testing Guide And Test Code Of Conduct](testing-and-test-code-of-conduct.md). + ## Version Flow Feature branches and `master` use different version modes because they solve different problems: feature builds need traceability to a commit, while `master` builds need stable semantic releases. diff --git a/docs/testing-and-test-code-of-conduct.md b/docs/testing-and-test-code-of-conduct.md index b3e185e..175d4b4 100644 --- a/docs/testing-and-test-code-of-conduct.md +++ b/docs/testing-and-test-code-of-conduct.md @@ -30,6 +30,8 @@ Windows and Linux use the same logical flow; only generator and compiler setup d ## Coverage Surface +**SonarCloud:** The CI Sonar scan reports coverage against `core/` paths that `ccm_core_tests` can execute. `ui_wx/` and the `app/` composition root are excluded from Sonar’s **coverage** calculation (`sonar.coverage.exclusions`) because they are not run under the doctest suite; UI behavior is covered by manual validation below. Other Sonar metrics still include those directories. + Current automated tests cover non-UI behavior, including: - filesystem naming and parsing behavior diff --git a/tests/AGENTS.md b/tests/AGENTS.md index 880c411..6b23b61 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -12,6 +12,7 @@ - `collection_service_tests.cpp` — `CollectionService` (uses inline `InMemoryRepo` + `StubImageStore`). - `config_service_tests.cpp` — `ConfigService` against `InMemoryFileSystem`. - `json_collection_repository_tests.cpp`, `json_set_repository_tests.cpp` — repository round-trips against `InMemoryFileSystem`. +- `local_image_store_tests.cpp` — `LocalImageStore` against `InMemoryFileSystem` + `ConfigService`: `copyIn` (extension preserved, missing source errors), `remove` (existing file deleted; absent path is a no-op), `resolvePath` layout under `dataStorage//images/`. - `set_service_tests.cpp` — `SetService` with `FakeSetSource` + `InMemSetRepo`. - `magic_set_source_tests.cpp` — `MagicSetSource::parseResponse` (Scryfall mapping). Drives `fetchAll` via `FixedHttpClient` fake. - `magic_card_preview_source_tests.cpp` — `MagicCardPreviewSource::buildSearchUrl` URL-encoding rules + `parseResponse` (`data[0].image_uris.normal`). Drives `fetchImageUrl` via `FixedHttpClient`. diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 168ad2e..0ee4426 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -12,6 +12,7 @@ add_executable(ccm_core_tests config_service_tests.cpp json_collection_repository_tests.cpp json_set_repository_tests.cpp + local_image_store_tests.cpp set_service_tests.cpp magic_set_source_tests.cpp magic_card_preview_source_tests.cpp diff --git a/tests/local_image_store_tests.cpp b/tests/local_image_store_tests.cpp new file mode 100644 index 0000000..22ad8f2 --- /dev/null +++ b/tests/local_image_store_tests.cpp @@ -0,0 +1,90 @@ +#include + +#include "ccm/infra/LocalImageStore.hpp" +#include "ccm/services/ConfigService.hpp" + +#include "fakes/InMemoryFileSystem.hpp" + +#include + +using namespace ccm; +using ccm::testing::InMemoryFileSystem; + +namespace { + +std::string dirNameForGame(Game g) { + switch (g) { + case Game::Magic: return "magic"; + case Game::Pokemon: return "pokemon"; + case Game::YuGiOh: return "yugioh"; + } + return "magic"; +} + +} // namespace + +TEST_SUITE("LocalImageStore") { + TEST_CASE("copyIn creates images directory and preserves source extension") { + InMemoryFileSystem mem; + ConfigService cfg{mem, "/app/config.json", "/coll"}; + REQUIRE(cfg.initialize().isOk()); + LocalImageStore store(mem, cfg, dirNameForGame); + + REQUIRE(mem.writeText("/incoming/card.PNG", "img-bytes").isOk()); + + auto r = store.copyIn(Game::Magic, "/incoming/card.PNG", "id001"); + REQUIRE(r.isOk()); + CHECK(r.value() == "id001.PNG"); + + const std::filesystem::path dest = + std::filesystem::path(cfg.current().dataStorage) / "magic" / "images" / "id001.PNG"; + REQUIRE(mem.exists(dest)); + auto body = mem.readText(dest); + REQUIRE(body.isOk()); + CHECK(body.value() == "img-bytes"); + } + + TEST_CASE("copyIn errors when source file is missing") { + InMemoryFileSystem mem; + ConfigService cfg{mem, "/app/config.json", "/coll"}; + REQUIRE(cfg.initialize().isOk()); + LocalImageStore store(mem, cfg, dirNameForGame); + + auto r = store.copyIn(Game::Pokemon, "/nope/missing.jpg", "x"); + REQUIRE(r.isErr()); + } + + TEST_CASE("remove deletes an existing image") { + InMemoryFileSystem mem; + ConfigService cfg{mem, "/app/config.json", "/coll"}; + REQUIRE(cfg.initialize().isOk()); + LocalImageStore store(mem, cfg, dirNameForGame); + + const std::filesystem::path imagePath = + std::filesystem::path(cfg.current().dataStorage) / "magic" / "images" / "a.png"; + REQUIRE(mem.ensureDirectory(imagePath.parent_path()).isOk()); + REQUIRE(mem.writeText(imagePath, "x").isOk()); + + REQUIRE(store.remove(Game::Magic, "a.png").isOk()); + CHECK_FALSE(mem.exists(imagePath)); + } + + TEST_CASE("remove succeeds when file is already absent") { + InMemoryFileSystem mem; + ConfigService cfg{mem, "/app/config.json", "/coll"}; + REQUIRE(cfg.initialize().isOk()); + LocalImageStore store(mem, cfg, dirNameForGame); + + REQUIRE(store.remove(Game::YuGiOh, "ghost.bin").isOk()); + } + + TEST_CASE("resolvePath joins data storage game images and filename") { + InMemoryFileSystem mem; + ConfigService cfg{mem, "/app/config.json", "/coll"}; + REQUIRE(cfg.initialize().isOk()); + LocalImageStore store(mem, cfg, dirNameForGame); + + const std::filesystem::path got = store.resolvePath(Game::Pokemon, "pic.jpg"); + CHECK(got.generic_string() == "/coll/pokemon/images/pic.jpg"); + } +} diff --git a/ui_wx/AGENTS.md b/ui_wx/AGENTS.md index 548d221..60d9e73 100644 --- a/ui_wx/AGENTS.md +++ b/ui_wx/AGENTS.md @@ -53,7 +53,7 @@ - Apply this rule consistently in shared templates (`BaseCardListPanel`, `BaseSelectedCardPanel`, `BaseCardEditDialog`) because a single implicit conversion in those bases affects every game view. 14. **Theme consistency rules (Windows):** - Treat dialog roots as `panelBg`, not a separate shade, otherwise label rows can look like mismatched darker boxes. - - Theme dialogs before `ShowModal()` with `applyThemeToWindowTree(...)`; this includes Settings, Create/Edit dialogs, image viewer, About, and custom popup dialogs. + - Theme dialogs before `ShowModal()` with `applyThemeToWindowTree(...)` (and root background/foreground colors as needed); this includes Settings, image viewer, About, and custom popup dialogs. Per-game **Add/Edit** flows use `themeModalDialog(wxDialog*, Theme)` from `Theme.hpp` so `MagicGameView` / `PokemonGameView` / `YuGiOhGameView` share one path instead of duplicating palette wiring. - Do not use native `wxMessageBox` / `wxAboutBox` for app-facing flows that must match dark mode. Use themed popup helpers (or a custom themed `wxDialog`) so body/buttons stay in sync with the app palette. - Center popup dialogs on the app window (`CentreOnParent()`) so confirmations/info boxes open relative to the current app window. - Include `wxSpinCtrl` in themed input controls (Amount field) or it will keep a mismatched native background. diff --git a/ui_wx/include/ccm/ui/Theme.hpp b/ui_wx/include/ccm/ui/Theme.hpp index 5b3b3b5..b5b0ea2 100644 --- a/ui_wx/include/ccm/ui/Theme.hpp +++ b/ui_wx/include/ccm/ui/Theme.hpp @@ -4,6 +4,7 @@ #include +class wxDialog; class wxWindow; class wxString; @@ -22,6 +23,7 @@ struct ThemePalette { ThemePalette paletteForTheme(Theme theme); Theme inferThemeFromWindow(const wxWindow* window); void applyThemeToWindowTree(wxWindow* root, const ThemePalette& palette, Theme theme); +void themeModalDialog(wxDialog* dlg, Theme theme); int showThemedMessageDialog(wxWindow* parent, const wxString& message, const wxString& caption, long style); int showThemedConfirmDialog(wxWindow* parent, const wxString& message, const wxString& caption); diff --git a/ui_wx/src/MagicGameView.cpp b/ui_wx/src/MagicGameView.cpp index eb3db0e..9dd6995 100644 --- a/ui_wx/src/MagicGameView.cpp +++ b/ui_wx/src/MagicGameView.cpp @@ -3,6 +3,7 @@ #include "ccm/ui/MagicCardEditDialog.hpp" #include "ccm/ui/MagicCardListPanel.hpp" #include "ccm/ui/MagicSelectedCardPanel.hpp" +#include "ccm/ui/Theme.hpp" #include @@ -94,13 +95,7 @@ void MagicGameView::onAddCard(wxWindow* parentWindow) { MagicCardEditDialog dlg(parentWindow, images_, sets_, EditMode::Create, fresh, &setsForDialog()); - { - const Theme currentTheme = config_.current().theme; - const ThemePalette palette = paletteForTheme(currentTheme); - applyThemeToWindowTree(&dlg, palette, currentTheme); - dlg.SetBackgroundColour(palette.panelBg); - dlg.SetForegroundColour(palette.text); - } + themeModalDialog(&dlg, config_.current().theme); if (dlg.ShowModal() != wxID_OK) return; auto added = collection_.add(Game::Magic, dlg.card()); @@ -139,13 +134,7 @@ void MagicGameView::onEditCard(wxWindow* parentWindow) { } MagicCardEditDialog dlg(parentWindow, images_, sets_, EditMode::Edit, *sel, &setsForDialog()); - { - const Theme currentTheme = config_.current().theme; - const ThemePalette palette = paletteForTheme(currentTheme); - applyThemeToWindowTree(&dlg, palette, currentTheme); - dlg.SetBackgroundColour(palette.panelBg); - dlg.SetForegroundColour(palette.text); - } + themeModalDialog(&dlg, config_.current().theme); if (dlg.ShowModal() != wxID_OK) return; auto updated = collection_.update(Game::Magic, dlg.card()); if (!updated) { diff --git a/ui_wx/src/PokemonGameView.cpp b/ui_wx/src/PokemonGameView.cpp index 344ccd5..5947e6a 100644 --- a/ui_wx/src/PokemonGameView.cpp +++ b/ui_wx/src/PokemonGameView.cpp @@ -3,6 +3,7 @@ #include "ccm/ui/PokemonCardEditDialog.hpp" #include "ccm/ui/PokemonCardListPanel.hpp" #include "ccm/ui/PokemonSelectedCardPanel.hpp" +#include "ccm/ui/Theme.hpp" #include @@ -91,13 +92,7 @@ void PokemonGameView::onAddCard(wxWindow* parentWindow) { PokemonCardEditDialog dlg(parentWindow, images_, sets_, EditMode::Create, fresh, &setsForDialog()); - { - const Theme currentTheme = config_.current().theme; - const ThemePalette palette = paletteForTheme(currentTheme); - applyThemeToWindowTree(&dlg, palette, currentTheme); - dlg.SetBackgroundColour(palette.panelBg); - dlg.SetForegroundColour(palette.text); - } + themeModalDialog(&dlg, config_.current().theme); if (dlg.ShowModal() != wxID_OK) return; auto added = collection_.add(Game::Pokemon, dlg.card()); @@ -136,13 +131,7 @@ void PokemonGameView::onEditCard(wxWindow* parentWindow) { } PokemonCardEditDialog dlg(parentWindow, images_, sets_, EditMode::Edit, *sel, &setsForDialog()); - { - const Theme currentTheme = config_.current().theme; - const ThemePalette palette = paletteForTheme(currentTheme); - applyThemeToWindowTree(&dlg, palette, currentTheme); - dlg.SetBackgroundColour(palette.panelBg); - dlg.SetForegroundColour(palette.text); - } + themeModalDialog(&dlg, config_.current().theme); if (dlg.ShowModal() != wxID_OK) return; auto updated = collection_.update(Game::Pokemon, dlg.card()); if (!updated) { diff --git a/ui_wx/src/Theme.cpp b/ui_wx/src/Theme.cpp index fad3983..966860d 100644 --- a/ui_wx/src/Theme.cpp +++ b/ui_wx/src/Theme.cpp @@ -609,6 +609,14 @@ void applyThemeToWindowTree(wxWindow* root, const ThemePalette& palette, Theme t } } +void themeModalDialog(wxDialog* dlg, Theme theme) { + if (dlg == nullptr) return; + const ThemePalette palette = paletteForTheme(theme); + applyThemeToWindowTree(dlg, palette, theme); + dlg->SetBackgroundColour(palette.panelBg); + dlg->SetForegroundColour(palette.text); +} + int showThemedMessageDialog(wxWindow* parent, const wxString& message, const wxString& caption, long style) { wxDialog dlg(parent, wxID_ANY, caption, wxDefaultPosition, wxDefaultSize, wxDEFAULT_DIALOG_STYLE | wxRESIZE_BORDER); diff --git a/ui_wx/src/YuGiOhGameView.cpp b/ui_wx/src/YuGiOhGameView.cpp index a16b645..601a8f1 100644 --- a/ui_wx/src/YuGiOhGameView.cpp +++ b/ui_wx/src/YuGiOhGameView.cpp @@ -3,6 +3,7 @@ #include "ccm/ui/YuGiOhCardEditDialog.hpp" #include "ccm/ui/YuGiOhCardListPanel.hpp" #include "ccm/ui/YuGiOhSelectedCardPanel.hpp" +#include "ccm/ui/Theme.hpp" #include @@ -100,13 +101,7 @@ void YuGiOhGameView::onAddCard(wxWindow* parentWindow) { YuGiOhCardEditDialog dlg(parentWindow, images_, sets_, cardPreview_, EditMode::Create, fresh, &setsForDialog()); - { - const Theme currentTheme = config_.current().theme; - const ThemePalette palette = paletteForTheme(currentTheme); - applyThemeToWindowTree(&dlg, palette, currentTheme); - dlg.SetBackgroundColour(palette.panelBg); - dlg.SetForegroundColour(palette.text); - } + themeModalDialog(&dlg, config_.current().theme); if (dlg.ShowModal() != wxID_OK) return; auto added = collection_.add(Game::YuGiOh, dlg.card()); @@ -145,13 +140,7 @@ void YuGiOhGameView::onEditCard(wxWindow* parentWindow) { } YuGiOhCardEditDialog dlg(parentWindow, images_, sets_, cardPreview_, EditMode::Edit, *sel, &setsForDialog()); - { - const Theme currentTheme = config_.current().theme; - const ThemePalette palette = paletteForTheme(currentTheme); - applyThemeToWindowTree(&dlg, palette, currentTheme); - dlg.SetBackgroundColour(palette.panelBg); - dlg.SetForegroundColour(palette.text); - } + themeModalDialog(&dlg, config_.current().theme); if (dlg.ShowModal() != wxID_OK) return; auto updated = collection_.update(Game::YuGiOh, dlg.card()); if (!updated) {