develop moved TempDir from beast:: to xrpl::, deleting
include/xrpl/beast/utility/temp_dir.h in favour of
include/xrpl/basics/FileUtilities.h. The merge kept this branch's
references to the old API, so the tree no longer compiled: the missing
header is a fatal include, and because DatabaseConfig_test.cpp lands in
the xrpld unity blob it broke the xrpld target itself, not just the tests.
Swap the include and drop the stale beast:: qualifier on 17 uses. The
three src/tests/libxrpl/nodestore files already include FileUtilities.h
and already spell TempDir unqualified elsewhere, so only the qualifier
was wrong there. DatabaseConfig_test.cpp needed the include as well; it
sits in namespace xrpl::node_store, so unqualified TempDir resolves to
xrpl::TempDir through the enclosing namespace.
Two further uses exist only on the sync-diagnostics tip and are fixed
there rather than here.
The fetch and store duration counters converted each sample to
microseconds before adding it, so any backend call finishing in under a
microsecond contributed zero. A warm nudb read answers in a few hundred
nanoseconds, so on fast hardware every read floored and the totals stayed
at zero no matter how many reads happened -- the same loss of resolution
the microsecond report was introduced to avoid, one decade lower.
Both accumulators now hold nanoseconds, the clock's own resolution, and
convert once in getFetchDurationUs() and getStoreDurationUs(). The public
accessors, the node_reads_duration_us and node_writes_duration_us JSON
fields, and the metrics that read them all keep microseconds, so nothing
downstream changes unit. storeDurationStats() takes the raw duration
instead of a pre-converted integer so no caller can round early, and
updateFetchMetrics() scales its microsecond input to match.
FetchReport::elapsed stays microseconds: it carries one fetch, not a
total, and that is the unit it declares. The reported sum is therefore the
accumulated total minus a sub-microsecond remainder per fetch, so the two
tests that asserted exact equality between them now assert that bound.
Both had assertions that depended on how fast the host reads; the bound
holds on any hardware.
The lane-attribution change added acquire_ledger_deferrals and
acquire_ledger_timeouts to the gauge but left this test expecting seven
labels, so it failed on every platform of both PRs: nine emitted against
seven expected. The exporters and the counters went in as separate
commits, which is how the test escaped the update.
The two new labels are asserted with values that differ from the all-lane
totals they are a subset of, so a counter wired to the wrong lane, or one
that ignored the flag and counted every lane, lands on the totals instead
and fails.
BEAST_EXPECT(getFetchDurationUs() > 0) failed on the macOS arm64 runner at
DatabaseConfig_test.cpp:879. It is an assertion about the host, not the
code: Database::fetchNodeObject() truncates each fetch to whole
microseconds, and a read served from NuDB's in-memory buckets can
genuinely measure under one microsecond. On a host fast enough for every
read to truncate, the total legitimately stays at zero.
Both fetch-duration checks now compare the accumulator against the total
the scheduler's fetch reports carried, via a CountingScheduler. Each
fetch is measured once and that one value feeds both, so the equality
holds at any host speed while being strictly stronger than '> 0': a
dropped fetch, a double-count, or the write member being returned all
break it. The same reasoning is already recorded in the GTest at
src/tests/libxrpl/nodestore/Database.cpp.
The store-duration '> 0' checks are left alone: those writes reach disk
and were not what failed.
All three were rejected by clang-tidy as errors:
- readability-math-missing-parentheses on 2 * kNumStored + kNumMissing
- misc-const-correctness on the never-mutated AcquireStats 'quiet'
- bugprone-unchecked-optional-access on the read_threads_running
dereferences: branching on BEAST_EXPECT's return value hides the
has_value() check from the analyser, so the guard is now a plain if and
the macro asserts separately.
The 22 metric label values on the nodestore_state gauge were asserted
nowhere, and the depthSum assertions could not tell the real
fetch_add(depth) from a fetch_add(1) that would silently zero every
derived queueing time.
Make the four observe* helpers and their ObserveFn sink public rather
than private, so the seam the header already claimed is actually
reachable. All four are static and read only their arguments, so this
exposes no object state; a friend declaration would have granted access
to every private member instead. The assertions live in the existing
Beast nodestore suite because src/test compiles into xrpld, which
contains MetricsRegistry.cpp, while xrpl_tests deliberately does not
when telemetry is enabled.
Each helper now has its exact emitted label set asserted, so a typo in
any literal fails instead of silently producing a disjoint series, and
each derived mean is asserted ABSENT on a fresh store -- a refactor to
value_or(0) would draw a believable flat zero on a latency axis and
otherwise pass everything.
Replace the concurrent write-stats test with one that forces genuine
overlap through a latch. NuDB holds one global mutex for the whole
insert and doInsert reads the depth before entering it, so a blocked
thread records a depth of at least 2; asserting depthSum strictly
exceeds insertCount therefore cannot be satisfied by a constant 1. The
old bounds admitted that bug at their floor.
Also: cover the std::nullopt branch on the two backends that exist in
every build, bound the store-duration accumulator by the wall clock,
drop four assertions that cannot fail, and correct two comments that
claimed coverage the tests do not have -- the duplicate-key test is not
the throwing path, because nudb reports key_exists without throwing,
and no test drives NodeStoreScheduler::onFetch.
- JobQueue_test: drop the unused <functional>, and make the two
read-only GaugeFixture instances const. The other two stay mutable
because they submit jobs through fixture.queue.
- DatabaseConfig_test: drop the unused SystemParameters.h and include
ByteUtilities.h for megabytes(), which was reached only transitively.
Regenerate ordering.txt for the resulting edges: test.unit_test arrives
with SuiteJournal.h, and xrpl.protocol leaves with SystemParameters.h.
beast::Journal has no default constructor, so holding one as a plain
member deleted the suite's own default constructor and the Beast
registration macro could not instantiate it. SuiteJournal takes the
suite, converts implicitly where a journal is expected, and routes log
output into the test report.
Also removes a leftover unused local in run(), which is a hard error
under the warnings-as-errors build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit e3c2f8279a)
The nodestore suites moved from Beast to GTest upstream, which deleted
src/test/nodestore/TestBase.h. DatabaseConfig_test stayed on Beast and
still derived its journal and batch helpers from that base, so once both
sides met in a merge it referenced three symbols that no longer existed.
It now carries its own copies, matching the current API: node object
types are NodeObjectType::Ledger rather than the old hotLEDGER spelling.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 05d01de81a)
