bank: a database per ctest case instead of one shared file, and a rounding that does not charge a whole minor unit for half of one (fixes #682, fixes #678, refs #685)

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")

examples/bank/gui/controllers/Format.hpp

@@ -2,6 +2,7 @@
 #pragma once
 
 #include <QString>
+#include <cmath>
 #include <cstdint>
 #include <optional>
 
@@ -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<std::int64_t> parseMinor(const QString& text, int decimals
     }
     // Reuse the core scale primitive so parse and format share one source.
     const auto scale = static_cast<double>(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<std::int64_t>(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::int64_t>(std::llround(scaled));
 }
 
 }  // namespace bankgui::fmt

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;

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<std::int64_t>(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());
+}

examples/bank/tests/gui/test_bank_gui_qml_behaviour.cpp

@@ -49,12 +49,10 @@
 #include <QVariantMap>
 #include <catch2/catch_test_macros.hpp>
 #include <cstdint>
-#include <filesystem>
 #include <initializer_list>
 #include <memory>
 #include <morph/core/bridge.hpp>
 #include <string>
-#include <system_error>
 
 #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};

examples/bank/tests/gui/test_bank_qml_surface.cpp

@@ -36,7 +36,6 @@
 #include <QString>
 #include <QStringList>
 #include <catch2/catch_test_macros.hpp>
-#include <filesystem>
 #include <initializer_list>
 #include <string>
 
@@ -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.

examples/bank/tests/test_account.cpp

@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #include <catch2/catch_test_macros.hpp>
-#include <filesystem>
 #include <morph/core/bridge.hpp>
 #include <string>
 
@@ -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<bank::AccountModel> 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<bank::AccountModel> accounts{app.bridge(), app.gui()};
     morph::bridge::BridgeHandler<bank::CustomerModel> accountsOwner{app.bridge(), app.gui()};

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<bank::AuthModel> 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<bank::AuthModel> auth{app.bridge(), app.gui()};
 
     auto anon = await(auth.execute(bank::dto::WhoAmI{}), app.guiLoop());

examples/bank/tests/test_budget.cpp

@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #include <catch2/catch_test_macros.hpp>
-#include <filesystem>
 #include <morph/core/bridge.hpp>
 #include <string>
 
@@ -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<bank::CustomerModel> accounts{app.bridge(), app.gui()};
     morph::bridge::BridgeHandler<bank::TransactionModel> txns{app.bridge(), app.gui()};

examples/bank/tests/test_card.cpp

@@ -1,7 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #include <catch2/catch_test_macros.hpp>
-#include <filesystem>
 #include <morph/core/bridge.hpp>
 #include <string>
 
@@ -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<bank::CustomerModel> accounts{app.bridge(), app.gui()};
     morph::bridge::BridgeHandler<bank::CardModel> cards{app.bridge(), app.gui()};