From 4d3f9d6f7b275b735c4b3ce315e005fef2f8d9ab Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:55:30 +0100 Subject: [PATCH] fix(tests): clear clang-tidy findings in the new nodestore metric tests The tests added on this branch tripped six checks under WarningsAsErrors. All of them are in test code introduced here. bugprone-unchecked-optional-access: gtest's ASSERT_TRUE returns an opaque AssertionResult, so the dataflow analysis cannot see that a following deref is guarded. Replaced with an explicit `if (!x.has_value()) FAIL()`, which the analysis does follow, or with a direct optional comparison where no deref is needed. Both keep the original assertion strength and add a reason string. readability-use-anyofallof: the two consteval helpers now use std::ranges::all_of. The static_assert still evaluates at compile time, verified by inverting the predicate and watching it fail. misc-const-correctness, misc-include-cleaner, modernize-use-designated-initializers: const on a never-mutated local, corrected include sets, and named fields on the Expected aggregate so its two adjacent bools cannot be transposed silently. No production code changes, and no NOLINT added. --- src/tests/libxrpl/ledger/AcquireStats.cpp | 2 +- src/tests/libxrpl/nodestore/Backend.cpp | 3 +- src/tests/libxrpl/nodestore/Database.cpp | 1 + src/tests/libxrpl/nodestore/NuDBFactory.cpp | 18 ++++++---- .../libxrpl/telemetry/MetricsRegistry.cpp | 36 +++++++++---------- .../telemetry/NodeStoreMetricNames.cpp | 12 +++---- 6 files changed, 38 insertions(+), 34 deletions(-) diff --git a/src/tests/libxrpl/ledger/AcquireStats.cpp b/src/tests/libxrpl/ledger/AcquireStats.cpp index defc031db8..4836c23fc5 100644 --- a/src/tests/libxrpl/ledger/AcquireStats.cpp +++ b/src/tests/libxrpl/ledger/AcquireStats.cpp @@ -30,7 +30,7 @@ using xrpl::AcquireStats; */ TEST(AcquireStatsTest, StartsAtZero) { - AcquireStats stats; + AcquireStats const stats; EXPECT_EQ(stats.getDeferrals(), 0u); EXPECT_EQ(stats.getTimeouts(), 0u); diff --git a/src/tests/libxrpl/nodestore/Backend.cpp b/src/tests/libxrpl/nodestore/Backend.cpp index a05ae2521d..cc1d515621 100644 --- a/src/tests/libxrpl/nodestore/Backend.cpp +++ b/src/tests/libxrpl/nodestore/Backend.cpp @@ -184,7 +184,8 @@ TEST_P(BackendTypeTest, write_stats_reported_only_when_measured) if (GetParam() == "nudb") { - ASSERT_TRUE(stats.has_value()); + if (!stats.has_value()) + FAIL() << "nudb must report write stats"; // Freshly opened and not yet written to. EXPECT_EQ(stats->insertCount, 0u); EXPECT_EQ(stats->concurrentWriters, 0u); diff --git a/src/tests/libxrpl/nodestore/Database.cpp b/src/tests/libxrpl/nodestore/Database.cpp index 70f06b42bc..298c906a23 100644 --- a/src/tests/libxrpl/nodestore/Database.cpp +++ b/src/tests/libxrpl/nodestore/Database.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include diff --git a/src/tests/libxrpl/nodestore/NuDBFactory.cpp b/src/tests/libxrpl/nodestore/NuDBFactory.cpp index e88356ed11..1153aa3749 100644 --- a/src/tests/libxrpl/nodestore/NuDBFactory.cpp +++ b/src/tests/libxrpl/nodestore/NuDBFactory.cpp @@ -279,7 +279,8 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert) // Before any write the stats exist but are all zero, and no writer is // in flight. auto const initial = backend->getWriteStats(); - ASSERT_TRUE(initial.has_value()); + if (!initial.has_value()) + FAIL() << "nudb must report write stats"; EXPECT_EQ(initial->insertCount, 0u); EXPECT_EQ(initial->insertTotalUs, 0u); EXPECT_EQ(initial->insertMaxUs, 0u); @@ -294,7 +295,8 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert) storeBatch(*backend, batch); auto const after = backend->getWriteStats(); - ASSERT_TRUE(after.has_value()); + if (!after.has_value()) + FAIL() << "nudb must report write stats after inserts"; EXPECT_EQ(after->insertCount, kFirstBatch); EXPECT_EQ(after->depthSum, kFirstBatch); EXPECT_GT(after->insertTotalUs, 0u); @@ -316,7 +318,8 @@ TEST(NuDBFactory, write_stats_accumulate_per_insert) storeBatch(*backend, more); auto const cumulative = backend->getWriteStats(); - ASSERT_TRUE(cumulative.has_value()); + if (!cumulative.has_value()) + FAIL() << "nudb must report write stats after a second batch"; EXPECT_EQ(cumulative->insertCount, kFirstBatch + kSecondBatch); EXPECT_EQ(cumulative->depthSum, kFirstBatch + kSecondBatch); EXPECT_EQ(cumulative->concurrentWriters, 0u); @@ -347,7 +350,8 @@ TEST(NuDBFactory, write_stats_count_duplicate_key_inserts) storeBatch(*backend, batch); auto const first = backend->getWriteStats(); - ASSERT_TRUE(first.has_value()); + if (!first.has_value()) + FAIL() << "nudb must report write stats"; ASSERT_EQ(first->insertCount, kBatchSize); // Re-storing the identical batch writes nothing new, but each call is @@ -355,7 +359,8 @@ TEST(NuDBFactory, write_stats_count_duplicate_key_inserts) storeBatch(*backend, batch); auto const second = backend->getWriteStats(); - ASSERT_TRUE(second.has_value()); + if (!second.has_value()) + FAIL() << "nudb must report write stats after re-storing"; EXPECT_EQ(second->insertCount, kBatchSize * 2); EXPECT_EQ(second->depthSum, kBatchSize * 2); // The depth returned to zero, so the early-return error path did not @@ -397,7 +402,8 @@ TEST(NuDBFactory, write_stats_observe_concurrent_writers) th.join(); auto const stats = backend->getWriteStats(); - ASSERT_TRUE(stats.has_value()); + if (!stats.has_value()) + FAIL() << "nudb must report write stats after concurrent inserts"; EXPECT_EQ(stats->insertCount, kThreads * kPerThread); EXPECT_GE(stats->depthSum, stats->insertCount); EXPECT_EQ(stats->concurrentWriters, 0u); diff --git a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp index afe7a4e1cf..490d622ab1 100644 --- a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp +++ b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp @@ -25,10 +25,12 @@ #include +#include #include #include #include #include +#include #include #include @@ -180,12 +182,9 @@ constexpr std::array kFoldToOtherHandlers = { consteval bool allPassThroughUnchanged() { - for (auto const name : kPassThroughHandlers) - { - if (MetricsRegistry::sanitiseHandler(name) != name) - return false; - } - return true; + return std::ranges::all_of(kPassThroughHandlers, [](auto const name) { + return MetricsRegistry::sanitiseHandler(name) == name; + }); } /** @@ -194,12 +193,9 @@ allPassThroughUnchanged() consteval bool allFoldToOther() { - for (auto const name : kFoldToOtherHandlers) - { - if (MetricsRegistry::sanitiseHandler(name) != MetricsRegistry::kHandlerOther) - return false; - } - return true; + return std::ranges::all_of(kFoldToOtherHandlers, [](auto const name) { + return MetricsRegistry::sanitiseHandler(name) == MetricsRegistry::kHandlerOther; + }); } // Compile-time guarantees. Duplicated at runtime below so a failure names @@ -337,7 +333,7 @@ TEST(MetricsRegistryScaledMean, zero_count_reports_absence_not_zero) // same numerator with one sample does produce a value, so the guard is // keyed on the count and is not rejecting everything. ASSERT_TRUE(Registry::scaledMean(500, 1).has_value()); - EXPECT_EQ(*Registry::scaledMean(500, 1), 500); + EXPECT_EQ(Registry::scaledMean(500, 1), std::optional{500}); } TEST(MetricsRegistryScaledMean, zero_total_over_real_samples_is_a_genuine_zero) @@ -348,7 +344,7 @@ TEST(MetricsRegistryScaledMean, zero_total_over_real_samples_is_a_genuine_zero) // not suppressed, or a working fast path looks like a dead one. auto const mean = Registry::scaledMean(0, 32); ASSERT_TRUE(mean.has_value()); - EXPECT_EQ(*mean, 0); + EXPECT_EQ(mean, std::optional{0}); } TEST(MetricsRegistryScaledMean, exact_means_are_computed_exactly) @@ -387,13 +383,15 @@ TEST(MetricsRegistryScaledMean, large_inputs_saturate_instead_of_wrapping) // would surface as a sudden dip to a healthy-looking small number, which // is the failure mode worth preventing. auto const saturated = Registry::scaledMean(kU64Max, 1); - ASSERT_TRUE(saturated.has_value()); + if (!saturated.has_value()) + FAIL() << "a saturated mean must still be reported"; EXPECT_EQ(*saturated, kI64Max); // Scaling must not overflow either: this quotient times 100 exceeds // int64 range, so it clamps rather than wraps negative. auto const scaled = Registry::scaledMean(kU64Max, 2, 100); - ASSERT_TRUE(scaled.has_value()); + if (!scaled.has_value()) + FAIL() << "a saturated scaled mean must still be reported"; EXPECT_EQ(*scaled, kI64Max); // Every result is non-negative; a negative latency or depth is @@ -404,11 +402,9 @@ TEST(MetricsRegistryScaledMean, large_inputs_saturate_instead_of_wrapping) // Just below the boundary the value is exact, not clamped, so the clamp // above is a real bound and not a blanket ceiling on everything. auto const exact = Registry::scaledMean(static_cast(kI64Max), 1); - ASSERT_TRUE(exact.has_value()); - EXPECT_EQ(*exact, kI64Max); + EXPECT_EQ(exact, std::optional{kI64Max}); auto const belowBoundary = Registry::scaledMean(1'000'000, 4, 100); - ASSERT_TRUE(belowBoundary.has_value()); - EXPECT_EQ(*belowBoundary, 25'000'000); + EXPECT_EQ(belowBoundary, std::optional{25'000'000}); } TEST(MetricsRegistryScaledMean, default_scale_is_one) diff --git a/src/tests/libxrpl/telemetry/NodeStoreMetricNames.cpp b/src/tests/libxrpl/telemetry/NodeStoreMetricNames.cpp index 7d10eb3938..0c558ef252 100644 --- a/src/tests/libxrpl/telemetry/NodeStoreMetricNames.cpp +++ b/src/tests/libxrpl/telemetry/NodeStoreMetricNames.cpp @@ -186,9 +186,9 @@ TEST(NodeStoreMetricNames, latency_guard_admits_zero_and_refuses_negatives) #include #include #include +#include #include #include -#include #include #include #include @@ -202,7 +202,7 @@ TEST(NodeStoreMetricNames, latency_guard_admits_zero_and_refuses_negatives) #include #include -#include +#include #include #include #include @@ -482,10 +482,10 @@ TEST(NodeStoreReadHistogram, the_two_labels_split_the_series_four_ways) // the assertion that a label mix-up would break: swapping two values would // put a sample in the wrong series and fail here. for (auto const& [isAsync, wasFound, value, bucket] : std::array{ - {{true, true, 3.0, 2}, - {true, false, 9.0, 3}, - {false, true, 40.0, 5}, - {false, false, 800.0, 9}}}) + {{.isAsync = true, .wasFound = true, .value = 3.0, .bucket = 2}, + {.isAsync = true, .wasFound = false, .value = 9.0, .bucket = 3}, + {.isAsync = false, .wasFound = true, .value = 40.0, .bucket = 5}, + {.isAsync = false, .wasFound = false, .value = 800.0, .bucket = 9}}}) { auto const* point = findPoint(points, isAsync, wasFound); ASSERT_NE(point, nullptr) << "missing series for fetch_type="