chore: Gtest migration followups second pass - #7888
Open
godexsoft wants to merge 4 commits into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Second follow-up PR clearing review comments left open on the merged test-migration PRs
(#7136, #7133, #7046, #7317, #7292). **Stacked on top of the first follow-up PR. **
Closes 10 more review threads with code and answers 4 without a change. Also splits
Buffer.cpp's single 230-line test into per-behaviour cases, taking the suite from867 to 887 tests.
Context of Change
base_uint.cppreadabilitybool caught+ try/catch blocks becameEXPECT_THAT(lambda, ::testing::ThrowsMessage<E>(msg)). That keeps the messageassertion the flag version was making, and gives a real failure message instead of
Value of: caught / Actual: false. First gmock matcher in this TU, so<gmock/gmock.h>was added; the target already links
GTest::gmock.single-letter locals in
TEST_F(BaseUintTest, base_uint):u→ascending,v→complement,w→rehashed,z→zero,t→expectedByte,d→byte,h→hasher,n→counter,x→xored,tmp→parsed, and the zero variants.Scoped to that function only —
u,vandhalso appear intestComparisonsandNonhash, where they mean different things.IntrusiveShared.cppstylebraces on 9 single-statement
if/forbodies..clang-tidysetsreadability-braces-around-statements.ShortStatementLines: 2, so one-line bodies areallowed unbraced, and clang-format leaves them alone.
lines between
TIBase's data members and its member functions.Benchmarks
auto* b = RegisterBenchmark(...)local is gone; the calls are chained like the otherregistration site 13 lines below. That removes the "can
bbe null?" question ratherthan answering it with an assert.
RunState::release()now also resetsavgPayload, and carries a comment answering "whynot assignments?": only swap-with-empty is guaranteed to hand back the capacity, and
these pools are large.
src/benchmarksadded to theEXCLUDElist incmake/XrplCov.cmake. Benchmarks are notrun by the coverage job, so their lines could only ever report as uncovered.
rocksdb entry in
backendConfigs()is now justtype=rocksdb, with no tuning keys, sothere is nothing left to drift. See "Before / After" below — this was worse than a
drift risk.
libxrpl
Buffer::fill(std::uint8_t)and used it at its one call site,intToVucinshamap/SHAMap.cpp. Covered by three new tests.Buffer.cppsplit into per-behaviour testsTEST_F(BufferTest, buffer)was a single ~230-line test covering construction, equality,copy, move, slice conversion and resize/clear in one body, with the distinctions carried by
bare
{ ... }blocks and comments. A failure anywhere reported as "BufferTest.bufferfailed".
It is now 18 named tests (
default_constructed_is_empty,move_assign_empty_to_non_empty,assignment_from_slice, …) plus the 3filltests. The shared sample data and referencebuffers moved into the fixture —
kData,kHalf,emptyBuffer,firstHalf,secondHalf,whole— which is what let each test shrink to the case it names.sane()stays a static fixture helper.
This is the #7046.2
theme applied to one file: "this should be applied more thoroughly to tests migrated to
gtest — we still have some places where there is one test that internally splits into
blocks of tests." The rest of the migrated suite still needs the same treatment.
One deliberate exception:
equality_compares_contentskeepsEXPECT_TRUE(a != b)ratherthan
EXPECT_NE, because the comparison operators are what is under test —EXPECT_EQ/EXPECT_NEwould route through the veryoperator==being verified. Theoriginal carried a comment to this effect; it is now explicit about the reason.
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Buffer::fill()is a new method onxrpl::Bufferininclude/xrpl/basics/Buffer.h. Purelyadditive — no existing signature, layout or behaviour changes.