fix: Use a single timestamp in server_info - #3156
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
godexsoft
left a comment
There was a problem hiding this comment.
Thank you, this is great! just one nit and i think this is good to go 👍
|
@snowyukitty I'm sorry for a slow PR review here, will you be able to take a look here? |
Addresses the review on XRPLF#3156: - `ClockType` is constrained by a new `util::SomeSystemClock` concept instead of being described in a doc comment. - The test clock moves out of `ServerInfoTests.cpp` into its own header, `tests/common/util/TestConstantClock.hpp`, so other suites can use it. - It is a class: the fixed instant is public on it, the call counter is private, and it exposes `callCount()` and `resetCounter()` rather than a writable static member.
|
Thanks — all four addressed in d02e270.
Rebased onto your develop merge. |
mathbunnyru
left a comment
There was a problem hiding this comment.
@snowyukitty, your PR was failing in clang-tidy; I fixed it.
May I ask you to sign your commit?
You can reset the diff on top of the latest develop (please make sure to update it), and make it a single signed commit.
|
Thanks for fixing the clang-tidy failure. I have seen the request to update to current |
server_infosampledsystem_clockonce when constructingInfoSection::timeand again when calculating
validated_ledger.age. If those samples straddled awhole-second boundary, the exact-age assertions could fail intermittently.
This change reuses the timestamp already stored in
output.info.timefor theage calculation. The clock remains
system_clockby default; making itinjectable lets the tests use fixed absolute ledger times and verify that
successful requests sample it exactly once. A regression test also preserves
the existing zero clamp for future close times.
Fix #2353
Testing
RPCServerInfoHandlerTest.*: 12 passed; repeated 100 times (1,200 executions)-Werror; affected Clang 19 translation units1.14.0