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.
This commit is contained in:
Pratik Mankawde
2026-07-28 14:55:30 +01:00
parent 8fab4ae507
commit 4d3f9d6f7b
6 changed files with 38 additions and 34 deletions

View File

@@ -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);

View File

@@ -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);

View File

@@ -20,6 +20,7 @@
#include <algorithm>
#include <atomic>
#include <chrono>
#include <cstddef>
#include <cstdint>
#include <memory>
#include <stdexcept>

View File

@@ -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);

View File

@@ -25,10 +25,12 @@
#include <gtest/gtest.h>
#include <algorithm>
#include <array>
#include <cstddef>
#include <cstdint>
#include <limits>
#include <optional>
#include <set>
#include <string_view>
@@ -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<std::int64_t>{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<std::int64_t>{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<std::uint64_t>(kI64Max), 1);
ASSERT_TRUE(exact.has_value());
EXPECT_EQ(*exact, kI64Max);
EXPECT_EQ(exact, std::optional<std::int64_t>{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<std::int64_t>{25'000'000});
}
TEST(MetricsRegistryScaledMean, default_scale_is_one)

View File

@@ -186,9 +186,9 @@ TEST(NodeStoreMetricNames, latency_guard_admits_zero_and_refuses_negatives)
#include <opentelemetry/exporters/memory/in_memory_metric_data.h>
#include <opentelemetry/exporters/memory/in_memory_metric_exporter_factory.h>
#include <opentelemetry/metrics/meter.h>
#include <opentelemetry/nostd/unique_ptr.h>
#include <opentelemetry/nostd/variant.h>
#include <opentelemetry/sdk/metrics/aggregation/aggregation_config.h>
#include <opentelemetry/sdk/metrics/data/metric_data.h>
#include <opentelemetry/sdk/metrics/data/point_data.h>
#include <opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader_factory.h>
#include <opentelemetry/sdk/metrics/export/periodic_exporting_metric_reader_options.h>
@@ -202,7 +202,7 @@ TEST(NodeStoreMetricNames, latency_guard_admits_zero_and_refuses_negatives)
#include <array>
#include <chrono>
#include <cstdint>
#include <cstddef>
#include <memory>
#include <string>
#include <utility>
@@ -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<Expected, 4>{
{{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="