From 6a0bb63a3b803689b7248fe3031f14db757f8ce4 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 21:00:48 +0200 Subject: [PATCH 1/2] bank/tests: give each ctest case its own SQLite file, so `ctest -j` stops failing 21 of 21 (fixes #682) `catch_discover_tests()` registers one ctest case per `TEST_CASE`, and ctest runs each as its own process. All fourteen `bank_tests` sources named one fixed path -- `temp_directory_path() / "morph_bank_tests.db"` -- and `ensureDatabase()` deleted and re-migrated it on the way in. That is correct under a serial ctest and nothing else. Measured on 7d4ca453, `ctest -j 12 -L bank`: 0% tests passed, 21 tests failed out of 21 HY000 (10) - [SQLite]disk I/O error (10) three runs out of three. CI runs ctest serially, which is the only reason this was latent rather than red. The ladder's remedy for the same hazard is `RESOURCE_LOCK morph_ladder_test_db` (cmake/morph_add_rung.cmake), which serialises the cases. Bank does not have to buy correctness with parallelism: no case here reads state another case wrote -- every process already began by wiping the schema -- so a path per process is behaviour-preserving where the lock is not free. `tests/unique_test_database.hpp` claims a directory under the temp directory with `create_directory`, whose `true` return is an exclusive claim against other processes, and removes it in the static's destructor. After, on the same tree and the same command, five runs out of five: 100% tests passed out of 21 Total Test time (real) = 1.09 sec against 5.7s for the same 21 cases serially -- which is what a `RESOURCE_LOCK` would have pinned every developer run to. `examples/bank/CMakeLists.txt` records that trade next to the three `catch_discover_tests()` calls, so the absent lock reads as a decision rather than the oversight it was. `bank_gui_qml_tests` had the same defect in miniature: two `TEST_CASE`s, one `morph_bank_gui_qml.db`, and a comment asserting that wiping "once per process" made them safe -- which is exactly what does not hold when the process is the case. Both GUI suites now take a private path too. All three suites: `ctest -j 12 -L bank` -> 100% of 28. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- examples/bank/CMakeLists.txt | 17 +++ examples/bank/tests/bank_test_support.hpp | 28 +++-- .../tests/gui/test_bank_gui_qml_behaviour.cpp | 22 +--- .../bank/tests/gui/test_bank_qml_surface.cpp | 11 +- examples/bank/tests/test_account.cpp | 15 +-- examples/bank/tests/test_auth.cpp | 13 +-- examples/bank/tests/test_budget.cpp | 12 +- examples/bank/tests/test_card.cpp | 12 +- examples/bank/tests/test_loan.cpp | 12 +- examples/bank/tests/test_notification.cpp | 12 +- examples/bank/tests/test_offline.cpp | 12 +- examples/bank/tests/test_payee.cpp | 14 +-- examples/bank/tests/test_payment.cpp | 12 +- examples/bank/tests/test_relations.cpp | 11 +- examples/bank/tests/test_remote.cpp | 8 +- examples/bank/tests/test_stateful_account.cpp | 14 +-- examples/bank/tests/test_statement.cpp | 12 +- examples/bank/tests/test_transaction.cpp | 10 +- examples/bank/tests/unique_test_database.hpp | 110 ++++++++++++++++++ 19 files changed, 175 insertions(+), 182 deletions(-) create mode 100644 examples/bank/tests/unique_test_database.hpp diff --git a/examples/bank/CMakeLists.txt b/examples/bank/CMakeLists.txt index 6e1c621de..f69d4f751 100644 --- a/examples/bank/CMakeLists.txt +++ b/examples/bank/CMakeLists.txt @@ -234,6 +234,23 @@ if(MORPH_BUILD_TESTS) # ADD_TAGS_AS_LABELS), so `-L bank` is the only selector available, # and CMakePresets.json's base-test sets `noTestsAction: error`, so a # label that stopped matching fails the leg instead of running nothing. + # + # No RESOURCE_LOCK on any of the three calls, and that is a decision + # rather than the oversight it was (morph#682). catch_discover_tests + # registers one ctest case per TEST_CASE and ctest runs each as its own + # process; bank's cases all named one fixed SQLite path under + # temp_directory_path() and each deleted it on the way in, so + # `ctest -j 12 -L bank` failed 21 of 21 with + # `HY000 (10) - [SQLite]disk I/O error (10)`. The ladder's remedy for + # the same hazard is RESOURCE_LOCK morph_ladder_test_db (see + # cmake/morph_add_rung.cmake's own comment on it), which serialises the + # cases. Bank does not need to buy correctness with parallelism: no + # case here reads state another case wrote -- every one already started + # from an empty schema -- so tests/unique_test_database.hpp gives each + # *process* a database directory of its own instead, and all 21 pass + # concurrently. Measured: 0/21 at -j 12 before, 21/21 at -j 12 after, + # in 1.1s against the same suite's 5.7s serial -- which is what a lock + # would have pinned every developer run to. catch_discover_tests(bank_tests DISCOVERY_MODE PRE_TEST PROPERTIES LABELS "bank") diff --git a/examples/bank/tests/bank_test_support.hpp b/examples/bank/tests/bank_test_support.hpp index 0dd95dfd6..638e8e7fc 100644 --- a/examples/bank/tests/bank_test_support.hpp +++ b/examples/bank/tests/bank_test_support.hpp @@ -15,24 +15,34 @@ #include "bank/db/database.hpp" #include "bank/db/entities.hpp" #include "bank/db/user_ops.hpp" +#include "unique_test_database.hpp" /// @file /// Shared helpers for the bank example tests. namespace bank::testing { -/// @brief Sets up the shared test database exactly once for the whole binary. +/// @brief The ODBC connection string every test in this process shares. /// -/// All tests run against one on-disk SQLite file (a single `:memory:` -/// connection cannot be shared across the per-model DataMappers). Migrations -/// are applied once; individual tests isolate themselves by using unique owner -/// principals rather than by wiping tables. +/// A path of this process's own, so two ctest cases running concurrently never +/// name the same SQLite file (morph#682). +/// +/// @return The connection string. +[[nodiscard]] inline const std::string& connectionString() { return uniqueDatabaseConnection(); } + +/// @brief Sets up this process's test database exactly once. +/// +/// All tests in one process run against one on-disk SQLite file (a single +/// `:memory:` connection cannot be shared across the per-model DataMappers). +/// Migrations are applied once; individual tests isolate themselves by using +/// unique owner principals rather than by wiping tables. +/// +/// The file is private to the process rather than a fixed path shared by every +/// bank test binary -- see unique_test_database.hpp for why, and for what a +/// fixed path cost under `ctest -j` (morph#682). inline void ensureDatabase() { static const bool once = [] { - const auto path = std::filesystem::temp_directory_path() / "morph_bank_tests.db"; - std::error_code err; - std::filesystem::remove(path, err); - bank::db::setup("DRIVER=SQLite3;Database=" + path.string()); + bank::db::setup(connectionString()); return true; }(); (void)once; diff --git a/examples/bank/tests/gui/test_bank_gui_qml_behaviour.cpp b/examples/bank/tests/gui/test_bank_gui_qml_behaviour.cpp index cf8139b2d..c6f9445ec 100644 --- a/examples/bank/tests/gui/test_bank_gui_qml_behaviour.cpp +++ b/examples/bank/tests/gui/test_bank_gui_qml_behaviour.cpp @@ -49,12 +49,10 @@ #include #include #include -#include #include #include #include #include -#include #include "BankClient.hpp" #include "Theme.hpp" @@ -69,27 +67,13 @@ #include "controllers/PayeeController.hpp" #include "controllers/TransactionController.hpp" #include "testkit/pump.hpp" +#include "unique_test_database.hpp" using morph::ladder::testkit::awaitQt; using morph::ladder::testkit::pumpUntil; namespace { -/// @brief A database of this suite's own -- its own file, not the one -/// `bank_tests` or the surface audit uses, so the binaries can run -/// concurrently -- wiped once per process rather than once per case, so -/// a later case cannot delete the file an earlier one still has open. -/// @return The ODBC connection string for it. -[[nodiscard]] std::string connectionString() { - static const std::string connection = [] { - const auto path = std::filesystem::temp_directory_path() / "morph_bank_gui_qml.db"; - std::error_code err; - std::filesystem::remove(path, err); - return "DRIVER=SQLite3;Database=" + path.string(); - }(); - return connection; -} - /// @brief URL of one of the GUI's shipped `.qml` files in the source tree. /// @param fileName Basename, e.g. `"MoveMoneyPage.qml"`. /// @return A `file:` URL the engine can load. @@ -159,7 +143,7 @@ namespace { // NOLINTNEXTLINE(readability-function-cognitive-complexity) TEST_CASE("MoveMoneyPage's picker keeps naming the account the next deposit will land in", "[bank][gui][qml][move-money]") { - bankgui::BankClient client{connectionString()}; + bankgui::BankClient client{bank::testing::uniqueDatabaseConnection()}; bankgui::AppController app{client}; app.registerUser(QStringLiteral("gui-move-money"), QStringLiteral("hunter2demo"), QStringLiteral("Picker")); @@ -267,7 +251,7 @@ TEST_CASE("MoveMoneyPage's picker keeps naming the account the next deposit will // run exits 1; with the directive back, the same diff is clean. // NOLINTNEXTLINE(readability-function-cognitive-complexity) TEST_CASE("Main.qml confirms a posted transaction and a paid bill in the toast", "[bank][gui][qml][toast]") { - bankgui::BankClient client{connectionString()}; + bankgui::BankClient client{bank::testing::uniqueDatabaseConnection()}; bankgui::AppController app{client}; bankgui::AccountController accountsController{client}; diff --git a/examples/bank/tests/gui/test_bank_qml_surface.cpp b/examples/bank/tests/gui/test_bank_qml_surface.cpp index b4b0d3e34..b0a33e466 100644 --- a/examples/bank/tests/gui/test_bank_qml_surface.cpp +++ b/examples/bank/tests/gui/test_bank_qml_surface.cpp @@ -36,7 +36,6 @@ #include #include #include -#include #include #include @@ -48,6 +47,7 @@ #include "controllers/PayeeController.hpp" #include "controllers/TransactionController.hpp" #include "testkit/qml_surface.hpp" +#include "unique_test_database.hpp" namespace { @@ -60,11 +60,10 @@ TEST_CASE("Every bank controller exposes exactly the surface gui/qml binds, and // A real BankClient, because every controller holds `BridgeHandler`s // constructed from one. Nothing here dispatches an action — the audit reads // metaobjects and text — but `BankClient`'s constructor runs the schema - // migrations, so it needs a database like any other bank test does. Its own - // file, not the one `bank_tests` shares, so the two binaries can run - // concurrently. - const auto dbPath = std::filesystem::temp_directory_path() / "morph_bank_qml_surface.db"; - bankgui::BankClient client{"DRIVER=SQLite3;Database=" + dbPath.string()}; + // migrations, so it needs a database like any other bank test does. A file + // private to this process, so no other ctest case — in this binary or + // another — can be unlinking it while this one has it open (morph#682). + bankgui::BankClient client{bank::testing::uniqueDatabaseConnection()}; // const: `QmlSurfaceAudit::bind` takes `const QObject&`, and nothing here // drives a controller -- the audit reads metaobjects and QML text. diff --git a/examples/bank/tests/test_account.cpp b/examples/bank/tests/test_account.cpp index 7c775506c..6ea018d2c 100644 --- a/examples/bank/tests/test_account.cpp +++ b/examples/bank/tests/test_account.cpp @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include #include @@ -15,18 +14,8 @@ using bank::testing::await; -namespace { - -/// Builds an App against the shared test DB and logs in @p principal. -std::string dbConnectionForTests() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("AccountModel opens, lists, fetches and closes accounts", "[account]") { - bank::app::App app{dbConnectionForTests()}; + bank::app::App app{bank::testing::connectionString()}; app.login("alice-account-basic"); morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; @@ -88,7 +77,7 @@ TEST_CASE("AccountModel opens, lists, fetches and closes accounts", "[account]") } TEST_CASE("AccountModel reports errors through onError", "[account]") { - bank::app::App app{dbConnectionForTests()}; + bank::app::App app{bank::testing::connectionString()}; app.login("bob-account-errors"); morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; morph::bridge::BridgeHandler accountsOwner{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_auth.cpp b/examples/bank/tests/test_auth.cpp index 5f685f69a..c14a0c3ea 100644 --- a/examples/bank/tests/test_auth.cpp +++ b/examples/bank/tests/test_auth.cpp @@ -13,17 +13,8 @@ using bank::testing::await; -namespace { - -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("AuthModel register/login/change-password flow", "[auth]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; morph::bridge::BridgeHandler auth{app.bridge(), app.gui()}; const std::string user = "carol-" + std::to_string(std::filesystem::hash_value("carol")); @@ -84,7 +75,7 @@ TEST_CASE("AuthModel register/login/change-password flow", "[auth]") { } TEST_CASE("AuthModel WhoAmI reflects the bridge session", "[auth]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; morph::bridge::BridgeHandler auth{app.bridge(), app.gui()}; auto anon = await(auth.execute(bank::dto::WhoAmI{}), app.guiLoop()); diff --git a/examples/bank/tests/test_budget.cpp b/examples/bank/tests/test_budget.cpp index ef60beb2b..9db16dc77 100644 --- a/examples/bank/tests/test_budget.cpp +++ b/examples/bank/tests/test_budget.cpp @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include #include @@ -17,17 +16,8 @@ using bank::testing::await; -namespace { - -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("BudgetModel upserts budgets and computes spending", "[budget]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("laura-budget"); morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; morph::bridge::BridgeHandler txns{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_card.cpp b/examples/bank/tests/test_card.cpp index 65dd5e545..b469ccaed 100644 --- a/examples/bank/tests/test_card.cpp +++ b/examples/bank/tests/test_card.cpp @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include #include @@ -16,17 +15,8 @@ using bank::testing::await; -namespace { - -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("CardModel issues and manages cards", "[card]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("judy-card"); morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; morph::bridge::BridgeHandler cards{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_loan.cpp b/examples/bank/tests/test_loan.cpp index 47328041a..99923d66d 100644 --- a/examples/bank/tests/test_loan.cpp +++ b/examples/bank/tests/test_loan.cpp @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include #include @@ -17,17 +16,8 @@ using bank::testing::await; -namespace { - -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("LoanModel disburses, schedules, and repays", "[loan]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("ken-loan"); morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; morph::bridge::BridgeHandler accountsOwner{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_notification.cpp b/examples/bank/tests/test_notification.cpp index 9ac8c12ae..9167d469c 100644 --- a/examples/bank/tests/test_notification.cpp +++ b/examples/bank/tests/test_notification.cpp @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include #include @@ -12,17 +11,8 @@ using bank::testing::await; -namespace { - -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("NotificationModel posts, lists, and marks read", "[notification]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("mike-notify"); morph::bridge::BridgeHandler notes{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_offline.cpp b/examples/bank/tests/test_offline.cpp index 6cce91b5e..efaf5e7a2 100644 --- a/examples/bank/tests/test_offline.cpp +++ b/examples/bank/tests/test_offline.cpp @@ -6,7 +6,6 @@ // queue and replays each action through the live bridge handler. #include -#include #include #include #include @@ -23,17 +22,8 @@ using bank::testing::await; -namespace { - -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("Offline deposits are queued and replayed on reconnect", "[offline]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("peter-offline"); morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; morph::bridge::BridgeHandler accountsOwner{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_payee.cpp b/examples/bank/tests/test_payee.cpp index d4eeaccc3..5adae6892 100644 --- a/examples/bank/tests/test_payee.cpp +++ b/examples/bank/tests/test_payee.cpp @@ -2,7 +2,6 @@ #include #include -#include #include #include @@ -15,17 +14,8 @@ using bank::testing::await; using bank::testing::waitUntil; -namespace { - -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("PayeeModel add/list/remove scoped to the owner", "[payee]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("grace-payee"); morph::bridge::BridgeHandler payees{app.bridge(), app.gui()}; @@ -58,7 +48,7 @@ TEST_CASE("PayeeModel add/list/remove scoped to the owner", "[payee]") { } TEST_CASE("PayeeModel notifies subscribers of the state it produces", "[payee][subscribe]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("heidi-form"); morph::bridge::BridgeHandler payees{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_payment.cpp b/examples/bank/tests/test_payment.cpp index 2591142e7..ce36c4612 100644 --- a/examples/bank/tests/test_payment.cpp +++ b/examples/bank/tests/test_payment.cpp @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include #include @@ -20,17 +19,8 @@ using bank::testing::await; -namespace { - -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("PaymentModel pays bills, schedules, and cancels", "[payment]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("ivan-pay"); morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; morph::bridge::BridgeHandler accountsOwner{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_relations.cpp b/examples/bank/tests/test_relations.cpp index a2f07821c..8b349cb8b 100644 --- a/examples/bank/tests/test_relations.cpp +++ b/examples/bank/tests/test_relations.cpp @@ -35,18 +35,9 @@ using bank::testing::await; -namespace { - -std::string dbConnectionForTests() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("ORM relations: BelongsTo navigation and HasMany inverses", "[relations]") { const std::string principal = "rel-user"; - bank::app::App app{dbConnectionForTests()}; + bank::app::App app{bank::testing::connectionString()}; app.login(principal); // provisions the users row morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_remote.cpp b/examples/bank/tests/test_remote.cpp index 927a671fa..0ddf846a0 100644 --- a/examples/bank/tests/test_remote.cpp +++ b/examples/bank/tests/test_remote.cpp @@ -7,7 +7,6 @@ // IAuthorizer that rejects one action type. #include -#include #include #include #include @@ -27,11 +26,6 @@ using bank::testing::await; namespace { -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - /// Authorizer that forbids closing accounts but allows everything else. struct NoCloseAuthorizer : morph::session::IAuthorizer { [[nodiscard]] bool authorize(const morph::session::Context& /*ctx*/, std::string_view /*model*/, @@ -59,7 +53,7 @@ struct NoCloseAuthorizer : morph::session::IAuthorizer { TEST_CASE("AccountModel runs unchanged over a remote backend", "[remote]") { bank::testing::ensureDatabase(); - (void)testConnection(); // ensure the shared DB is configured + (void)bank::testing::connectionString(); // ensure the shared DB is configured morph::exec::ThreadPoolExecutor serverPool{2}; morph::exec::MainThreadExecutor gui; diff --git a/examples/bank/tests/test_stateful_account.cpp b/examples/bank/tests/test_stateful_account.cpp index 328e9b96d..4f4ae0b54 100644 --- a/examples/bank/tests/test_stateful_account.cpp +++ b/examples/bank/tests/test_stateful_account.cpp @@ -11,7 +11,6 @@ #include #include -#include #include #include @@ -30,11 +29,6 @@ using morph::bridge::BridgeHandler; namespace { -std::string statefulTestConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - /// Opens a checking account for the logged-in principal and returns its id. std::int64_t openChecking(bank::app::App& app, BridgeHandler& customer) { auto info = await(customer.execute(bank::dto::OpenAccount{ @@ -49,7 +43,7 @@ std::int64_t openChecking(bank::app::App& app, BridgeHandler customer{app.bridge(), app.gui()}; @@ -71,7 +65,7 @@ TEST_CASE("two shared handlers on one account reach one instance", "[stateful-ac } TEST_CASE("a cached account re-hydrates after another model moves money", "[stateful-account]") { - bank::app::App app{statefulTestConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("tess-stale-cache"); BridgeHandler customer{app.bridge(), app.gui()}; @@ -92,7 +86,7 @@ TEST_CASE("a cached account re-hydrates after another model moves money", "[stat } TEST_CASE("a plain account handler keeps its own instance", "[stateful-account]") { - bank::app::App app{statefulTestConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("percy-private-instance"); BridgeHandler customer{app.bridge(), app.gui()}; @@ -110,7 +104,7 @@ TEST_CASE("a plain account handler keeps its own instance", "[stateful-account]" } TEST_CASE("closing through the cached instance still enforces the zero-balance rule", "[stateful-account]") { - bank::app::App app{statefulTestConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("cass-close-guard"); BridgeHandler customer{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_statement.cpp b/examples/bank/tests/test_statement.cpp index 64a2ef1a9..9b2f90d34 100644 --- a/examples/bank/tests/test_statement.cpp +++ b/examples/bank/tests/test_statement.cpp @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include #include @@ -16,17 +15,8 @@ using bank::testing::await; -namespace { - -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - -} // namespace - TEST_CASE("StatementModel aggregates credits and debits across accounts", "[statement]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("nina-stmt"); morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; morph::bridge::BridgeHandler txns{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/test_transaction.cpp b/examples/bank/tests/test_transaction.cpp index b1884ebbf..12a5f395f 100644 --- a/examples/bank/tests/test_transaction.cpp +++ b/examples/bank/tests/test_transaction.cpp @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include #include @@ -19,11 +18,6 @@ using bank::testing::await; namespace { -std::string testConnection() { - bank::testing::ensureDatabase(); - return "DRIVER=SQLite3;Database=" + (std::filesystem::temp_directory_path() / "morph_bank_tests.db").string(); -} - /// Opens a fresh checking account in the given currency and returns its id. std::int64_t openAccount(bank::app::App& app, morph::bridge::BridgeHandler& customer, bank::Currency currency = bank::Currency::USD) { @@ -39,7 +33,7 @@ std::int64_t openAccount(bank::app::App& app, morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; morph::bridge::BridgeHandler accountsOwner{app.bridge(), app.gui()}; @@ -78,7 +72,7 @@ TEST_CASE("TransactionModel deposit / withdraw adjust balances and ledger", "[tr } TEST_CASE("TransactionModel transfer is atomic and balance-preserving", "[transaction]") { - bank::app::App app{testConnection()}; + bank::app::App app{bank::testing::connectionString()}; app.login("frank-transfer"); morph::bridge::BridgeHandler accounts{app.bridge(), app.gui()}; morph::bridge::BridgeHandler accountsOwner{app.bridge(), app.gui()}; diff --git a/examples/bank/tests/unique_test_database.hpp b/examples/bank/tests/unique_test_database.hpp new file mode 100644 index 000000000..147bd29c2 --- /dev/null +++ b/examples/bank/tests/unique_test_database.hpp @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: Apache-2.0 +#pragma once + +#include +#include +#include +#include +#include +#include +#include + +/// @file +/// A SQLite database file that belongs to one process and no other. +/// +/// `catch_discover_tests()` registers **one ctest case per `TEST_CASE`**, and +/// ctest runs each of those as its own process. Every bank test binary used to +/// name its database by a fixed path under `temp_directory_path()`, and each +/// process deleted and re-migrated that file on the way in — which is correct +/// under a serial `ctest` and nothing else. Under `ctest -j`, 21 processes +/// unlinked and re-created one file concurrently and 21 of 21 cases failed with +/// `HY000 (10) - [SQLite]disk I/O error (10)` (morph#682). +/// +/// The remedy the ladder took for the same hazard is `RESOURCE_LOCK` (see +/// `cmake/morph_add_rung.cmake`), which serialises the cases instead. Bank does +/// not need to pay that: nothing here shares state *between* cases — each one +/// already started from an empty schema, and tests isolate themselves by owner +/// principal — so giving each process a path of its own restores correctness +/// without giving up the parallelism. + +namespace bank::testing { + +namespace detail { + +/// @brief Creates a directory under the system temp directory that no other +/// process holds. +/// +/// `std::filesystem::create_directory` reports whether *this* call created the +/// directory, and the underlying `mkdir`/`CreateDirectoryW` is atomic against +/// other processes, so a `true` return is an exclusive claim — which a +/// "generate a name, then check whether it exists" scheme would not be. +/// +/// @return The path of the freshly created, exclusively owned directory. +/// @throws std::runtime_error if no candidate name could be claimed. +[[nodiscard]] inline std::filesystem::path createExclusiveTempDirectory() { + std::random_device entropy; + const std::filesystem::path base = std::filesystem::temp_directory_path(); + std::error_code lastError; + for (int attempt = 0; attempt < 64; ++attempt) { + const auto token = (static_cast(entropy()) << 32U) | static_cast(entropy()); + const std::filesystem::path candidate = base / std::format("morph_bank_tests-{:016x}", token); + std::error_code err; + if (std::filesystem::create_directory(candidate, err)) { + return candidate; + } + lastError = err; + } + throw std::runtime_error("bank tests: could not create a private database directory under " + base.string() + + ": " + lastError.message()); +} + +/// @brief Owns the private directory for the lifetime of the process and +/// removes it on the way out. +class PrivateDatabase { +public: + /// @brief Claims a directory of this process's own and names a database + /// file inside it. + PrivateDatabase() + : _directory(createExclusiveTempDirectory()), + _connection("DRIVER=SQLite3;Database=" + (_directory / "bank.db").string()) {} + + PrivateDatabase(const PrivateDatabase&) = delete; + PrivateDatabase(PrivateDatabase&&) = delete; + PrivateDatabase& operator=(const PrivateDatabase&) = delete; + PrivateDatabase& operator=(PrivateDatabase&&) = delete; + + /// @brief Deletes the directory and everything SQLite left in it. + /// + /// Best effort: a process killed outright (a sanitizer `halt_on_error` + /// abort, say) leaves the directory behind, which is a stale temp + /// directory and not a failed run. + ~PrivateDatabase() { + std::error_code err; + std::filesystem::remove_all(_directory, err); + } + + /// @brief The ODBC connection string for the private database file. + /// @return A connection string naming a path no other process uses. + [[nodiscard]] const std::string& connection() const noexcept { return _connection; } + +private: + std::filesystem::path _directory; + std::string _connection; +}; + +} // namespace detail + +/// @brief The ODBC connection string for this process's own SQLite file. +/// +/// Stable for the life of the process and different in every other process, so +/// two ctest cases running concurrently cannot touch the same file. The file +/// itself is not created here — the caller's first connection does that, and +/// the containing directory is already empty. +/// +/// @return The connection string, owned by a function-local static. +[[nodiscard]] inline const std::string& uniqueDatabaseConnection() { + static const detail::PrivateDatabase database; + return database.connection(); +} + +} // namespace bank::testing From f8e610e22f257f80e49d2d54113bbed759764db1 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 21:09:34 +0200 Subject: [PATCH 2/2] bank/gui: round to nearest with llround, so an amount just under half a minor unit is not charged a whole one (fixes #678) `parseMinor` computed `(major * scale) + 0.5` and truncated. That is not "round to nearest": for the double immediately below one half the sum is not representable and rounds **up** to exactly 1.0, so the truncation returns 1 for a value that is below half a minor unit. The witness is typeable, not constructed: "0.004999999999999999" -> scaled = 0.49999999999999994 -> minor = 1 x = 0.49999999999999994449 x < 0.5 = true x + 0.5 = 1 (int64)(x + 0.5) = 1 <- returned std::llround(x) = 0 <- correct `std::llround` is the fix, and it also removes the construct `bugprone-incorrect-roundings` was pointing at rather than moving it somewhere the check no longer matches. morph#663's guard order survives, which is the one property this change rests on. The bound now applies to the *unrounded* scaled value, which is the stronger check: every `double` strictly below 2^63 is at most 2^63-1024, so `llround` of anything that passes the guard lands inside `std::int64_t` with 1023 to spare, and the accept/reject edge does not move -- doubles near 2^63 are 1024 apart, so the `+ 0.5` never crossed it either. `nan` is still rejected by the negated comparison, and the existing morph#663 cases still pass. The new case uses the witness. `0.005` and `0.004` give the same answer before and after and would have pinned nothing; the pre-existing `0.005` case stays, now labelled as the half-way input that must keep rounding away from zero. Measured on this branch, before the Format.hpp change and after: before: FAILED: CHECK( parseMinor("0.004999999999999999") == 0 ) with expansion: {?} == 0 assertions: 3 | 1 passed | 2 failed after: All tests passed (20 assertions in 5 test cases) Scope, plainly: this is one minor unit on a pathological input. It is not morph#663 -- that was undefined behaviour on ordinary input -- and it is worth fixing because the fix is smaller than the argument, not because it is dangerous. Verified under `clang-ubsan` with bank + bank GUI, the configuration morph#683's bank-sanitizers job uses: `ctest -L bank` -> 100% of 29, and check_sanitizer_instrumentation reports 9 of 9 binaries instrumented. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- examples/bank/gui/controllers/Format.hpp | 42 +++++++++++++------ .../bank/tests/gui/test_bank_gui_format.cpp | 36 +++++++++++++++- 2 files changed, 64 insertions(+), 14 deletions(-) diff --git a/examples/bank/gui/controllers/Format.hpp b/examples/bank/gui/controllers/Format.hpp index 21702986c..d23bca03d 100644 --- a/examples/bank/gui/controllers/Format.hpp +++ b/examples/bank/gui/controllers/Format.hpp @@ -2,6 +2,7 @@ #pragma once #include +#include #include #include @@ -83,14 +84,19 @@ inline constexpr double kMinorUnitsBound = 0x1p63; /// "reject this input", so the out-of-range cases join the ones that were /// already rejected rather than needing new handling. /// -/// The range check is what stops the conversion below being undefined -/// behaviour: converting a `double` whose truncated value is outside the -/// destination's range is UB ([conv.fpint]), and `QString::toDouble` happily -/// accepts `1e30` from a QML text field with no validator (morph#663). The -/// check is on the *scaled* value rather than on @p text's value, because only -/// the scaled value is what gets converted -- `double` arithmetic itself -/// cannot trap here, so computing it first costs nothing and removes the need -/// to reason about how dividing the bound by @p scale rounds. +/// The range check is what makes the rounding below defined: `std::llround` +/// on a value whose result is outside `long long` raises a domain error and +/// returns an unspecified value, exactly as converting one directly was UB +/// ([conv.fpint]) before it, and `QString::toDouble` happily accepts `1e30` +/// from a QML text field with no validator (morph#663). The check is on the +/// *scaled* value rather than on @p text's value, because only the scaled +/// value is what gets rounded -- `double` arithmetic itself cannot trap here, +/// so computing it first costs nothing and removes the need to reason about +/// how dividing the bound by @p scale rounds. +/// +/// Rounding is to nearest, halves away from zero. It is `std::llround` rather +/// than a `+ 0.5` and a truncation, which is not the same function: the two +/// disagree on the double immediately below one half (morph#678). /// /// @param text the user-entered amount, in major units /// @param decimals the number of minor-unit digits of the target currency @@ -104,14 +110,26 @@ inline std::optional parseMinor(const QString& text, int decimals } // Reuse the core scale primitive so parse and format share one source. const auto scale = static_cast(bank::pow10i(decimals)); - const double minor = (major * scale) + 0.5; - // Negated rather than written as `minor >= kMinorUnitsBound`, so that a + const double scaled = major * scale; + // Negated rather than written as `scaled >= kMinorUnitsBound`, so that a // NaN -- which compares false against everything, and which reaches here // because `nan < 0.0` is false -- is rejected rather than let through. - if (!(minor < kMinorUnitsBound)) { + // + // The guard is still what makes the line below defined, and it still runs + // first (morph#663). It bounds the *unrounded* value, which is the + // stronger of the two: every `double` strictly below 2^63 is at most + // 2^63-1024, so its rounding is inside `std::int64_t` with room to spare, + // and the bound stays the one form that is exact. + if (!(scaled < kMinorUnitsBound)) { return std::nullopt; } - return static_cast(minor); + // `std::llround`, not `(major * scale) + 0.5` truncated: the two disagree + // on the double immediately below one half. 0.49999999999999994 + 0.5 is + // exactly 1.0 in IEEE-754 -- the sum is not representable and rounds up -- + // so truncating it charged a whole minor unit for an amount below half of + // one (morph#678). `llround` rounds to nearest with halves away from zero, + // which is what the `+ 0.5` was reaching for. + return static_cast(std::llround(scaled)); } } // namespace bankgui::fmt diff --git a/examples/bank/tests/gui/test_bank_gui_format.cpp b/examples/bank/tests/gui/test_bank_gui_format.cpp index 81da71f0f..a04518e75 100644 --- a/examples/bank/tests/gui/test_bank_gui_format.cpp +++ b/examples/bank/tests/gui/test_bank_gui_format.cpp @@ -40,8 +40,10 @@ TEST_CASE("parseMinor turns well-formed amounts into minor units", "[bank][gui][ CHECK(parseMinor(QStringLiteral("12.34")) == 1234); CHECK(parseMinor(QStringLiteral("0")) == 0); CHECK(parseMinor(QStringLiteral(" 7.5 ")) == 750); - // Rounds to nearest rather than truncating, which is what the `+ 0.5` - // does; pinned here only so that a future change to it is a visible one. + // Rounds to nearest rather than truncating, with a half going away from + // zero. This input alone does not distinguish `std::llround` from the + // `+ 0.5` it replaced -- both give 1 -- which is the whole point of the + // last case in this file. CHECK(parseMinor(QStringLiteral("0.005")) == 1); // `decimals` comes from the selected currency (JPY has none). CHECK(parseMinor(QStringLiteral("1200"), 0) == 1200); @@ -91,3 +93,33 @@ TEST_CASE("parseMinor's ceiling is the int64 range, not an arbitrary cap", "[ban // between them rather than somewhere arbitrary below. CHECK_FALSE(parseMinor(QStringLiteral("920000000000000000")).has_value()); } + +// The morph#678 regression. `static_cast(x + 0.5)` is not +// "round to nearest": for the double immediately below 0.5, adding 0.5 rounds +// *up* to exactly 1.0 in IEEE-754, and the truncating cast then yields 1 for a +// value that is below half a minor unit. +// +// The witness has to be an input where the two disagree -- `0.005` and `0.004` +// give the same answer either way and would pin nothing. The first case in +// this file keeps `0.005` for exactly that reason: it is the half-way input +// that must still round away from zero, and it does under both. +TEST_CASE("parseMinor rounds a value just below half a minor unit down", "[bank][gui][format]") { + // "0.004999999999999999" scales to 0.49999999999999994, the largest + // double below 0.5: + // + // x = 0.49999999999999994449 + // x < 0.5 = true + // x + 0.5 = 1 + // (int64)(x + 0.5) = 1 <- what this function returned + // std::llround(x) = 0 + // + // Not a constructed bit pattern: a decimal string short enough to type + // into the amount field, through `QString::toDouble`. + CHECK(parseMinor(QStringLiteral("0.004999999999999999")) == 0); + CHECK(parseMinor(QStringLiteral("0.0049999999999999994")) == 0); + + // morph#663's bound still comes first. Rounding a value outside the int64 + // range is no better defined than casting one, so an amount that cannot + // fit has to be rejected before it is rounded, not after. + CHECK_FALSE(parseMinor(QStringLiteral("1e30")).has_value()); +}