Increase test coverage

.codecov.yml

@@ -1,6 +1,37 @@
+codecov:
+ require_ci_to_pass: true
+
+comment:
+ behavior: default
+ layout: reach,diff,flags,tree,reach
+ show_carryforward_flags: false
+
 coverage:
+ range: "60..80"
+ precision: 1
+ round: nearest
  status:
  project:
  default:
  target: 60%
  threshold: 2%
+ patch:
+ default:
+ target: auto
+ threshold: 2%
+ changes: false
+
+github_checks:
+ annotations: true
+
+parsers:
+ cobertura:
+ partials_as_hits: true
+ handle_missing_conditions : true
+
+slack_app: false
+
+ignore:
+ - "src/test/"
+ - "src/ripple/beast/test/"
+ - "src/ripple/beast/unit_test/"

.github/workflows/nix.yml

@@ -179,6 +179,8 @@ jobs:
  run: |
  mkdir -p ~/.conan
  tar -xzf conan.tar -C ~/.conan
+ - name: install gcovr
+ run: pip install "gcovr>=7,<8"
  - name: check environment
  run: |
  echo ${PATH} | tr ':' '\n'
@@ -207,7 +209,7 @@ jobs:
  -DCMAKE_CXX_FLAGS="-O0"
  -DCMAKE_C_FLAGS="-O0"
  cmake-target: coverage
- - name: build
+ - name: move coverage report
  shell: bash
  run: |
  mv "${build_dir}/coverage.xml" ./
@@ -218,13 +220,15 @@ jobs:
  path: coverage.xml
  retention-days: 30
  - name: upload coverage report
- uses: wandalen/wretry.action@v1.3.0
+ uses: wandalen/wretry.action@v1.4.10
  with:
- action: codecov/codecov-action@v4
+ action: codecov/codecov-action@v4.3.0
  with: |
  files: coverage.xml
  fail_ci_if_error: true
+ disable_search: true
  verbose: true
+ plugin: noop
  token: ${{ secrets.CODECOV_TOKEN }}
  attempt_limit: 5
- attempt_delay: 35000 # in milliseconds
+ attempt_delay: 210000 # in milliseconds

Builds/CMake/CodeCoverage.cmake

@@ -95,6 +95,9 @@
 # - replace both functions setup_target_for_coverage_gcovr_* with a single setup_target_for_coverage_gcovr
 # - add support for all gcovr output formats
 #
+# 2024-04-03, Bronek Kozicki
+# - add support for output formats: jacoco, clover, lcov
+#
 # USAGE:
 #
 # 1. Copy this file into your cmake modules path.
@@ -256,10 +259,10 @@ endif()
 # BASE_DIRECTORY "../" # Base directory for report
 # # (defaults to PROJECT_SOURCE_DIR)
 # FORMAT "cobertura" # Output format, one of:
-# # xml cobertura sonarqube json-summary
-# # json-details coveralls csv txt
-# # html-single html-nested html-details
-# # (xml is an alias to cobertura;
+# # xml cobertura sonarqube jacoco clover
+# # json-summary json-details coveralls csv
+# # txt html-single html-nested html-details
+# # lcov (xml is an alias to cobertura;
 # # if no format is set, defaults to xml)
 # EXCLUDE "src/dir1/*" "src/dir2/*" # Patterns to exclude (can be relative
 # # to BASE_DIRECTORY, with CMake 3.4+)
@@ -308,6 +311,8 @@ function(setup_target_for_coverage_gcovr)
  set(GCOVR_OUTPUT_FILE ${Coverage_NAME}.txt)
  elseif(Coverage_FORMAT STREQUAL "csv")
  set(GCOVR_OUTPUT_FILE ${Coverage_NAME}.csv)
+ elseif(Coverage_FORMAT STREQUAL "lcov")
+ set(GCOVR_OUTPUT_FILE ${Coverage_NAME}.lcov)
  else()
  set(GCOVR_OUTPUT_FILE ${Coverage_NAME}.xml)
  endif()
@@ -320,6 +325,14 @@ function(setup_target_for_coverage_gcovr)
  set(Coverage_FORMAT cobertura) # overwrite xml
  elseif(Coverage_FORMAT STREQUAL "sonarqube")
  list(APPEND GCOVR_ADDITIONAL_ARGS --sonarqube "${GCOVR_OUTPUT_FILE}" )
+ elseif(Coverage_FORMAT STREQUAL "jacoco")
+ list(APPEND GCOVR_ADDITIONAL_ARGS --jacoco "${GCOVR_OUTPUT_FILE}" )
+ list(APPEND GCOVR_ADDITIONAL_ARGS --jacoco-pretty )
+ elseif(Coverage_FORMAT STREQUAL "clover")
+ list(APPEND GCOVR_ADDITIONAL_ARGS --clover "${GCOVR_OUTPUT_FILE}" )
+ list(APPEND GCOVR_ADDITIONAL_ARGS --clover-pretty )
+ elseif(Coverage_FORMAT STREQUAL "lcov")
+ list(APPEND GCOVR_ADDITIONAL_ARGS --lcov "${GCOVR_OUTPUT_FILE}" )
  elseif(Coverage_FORMAT STREQUAL "json-summary")
  list(APPEND GCOVR_ADDITIONAL_ARGS --json-summary "${GCOVR_OUTPUT_FILE}" )
  list(APPEND GCOVR_ADDITIONAL_ARGS --json-summary-pretty)

src/ripple/app/misc/impl/AMMUtils.cpp

@@ -60,9 +60,12 @@ ammHolds(
  *optIssue2,
  std::make_optional(std::make_pair(issue1, issue2))))
  {
+ // This error can only be hit if the AMM is corrupted
+ // LCOV_EXCL_START
  JLOG(j.debug()) << "ammHolds: Invalid optIssue1 or optIssue2 "
  << *optIssue1 << " " << *optIssue2;
  return std::nullopt;
+ // LCOV_EXCL_STOP
  }
  return std::make_optional(std::make_pair(*optIssue1, *optIssue2));
  }
@@ -74,17 +77,21 @@ ammHolds(
  return std::make_optional(std::make_pair(issue1, issue2));
  else if (checkIssue == issue2)
  return std::make_optional(std::make_pair(issue2, issue1));
+ // Unreachable unless AMM corrupted.
+ // LCOV_EXCL_START
  JLOG(j.debug())
  << "ammHolds: Invalid " << label << " " << checkIssue;
  return std::nullopt;
+ // LCOV_EXCL_STOP
  };
  if (optIssue1)
  {
  return singleIssue(*optIssue1, "optIssue1");
  }
  else if (optIssue2)
  {
- return singleIssue(*optIssue2, "optIssue2");
+ // Cannot have Amount2 without Amount.
+ return singleIssue(*optIssue2, "optIssue2"); // LCOV_EXCL_LINE
  }
  return std::make_optional(std::make_pair(issue1, issue2));
  }();
@@ -210,19 +217,23 @@ deleteAMMTrustLines(
  // Should only have the trustlines
  if (nodeType != LedgerEntryType::ltRIPPLE_STATE)
  {
+ // LCOV_EXCL_START
  JLOG(j.error())
  << "deleteAMMTrustLines: deleting non-trustline "
  << nodeType;
  return {tecINTERNAL, SkipEntry::No};
+ // LCOV_EXCL_STOP
  }
 
  // Trustlines must have zero balance
  if (sleItem->getFieldAmount(sfBalance) != beast::zero)
  {
+ // LCOV_EXCL_START
  JLOG(j.error())
  << "deleteAMMTrustLines: deleting trustline with "
  "non-zero balance.";
  return {tecINTERNAL, SkipEntry::No};
+ // LCOV_EXCL_STOP
  }
 
  return {
@@ -243,18 +254,22 @@ deleteAMMAccount(
  auto ammSle = sb.peek(keylet::amm(asset, asset2));
  if (!ammSle)
  {
+ // LCOV_EXCL_START
  JLOG(j.error()) << "deleteAMMAccount: AMM object does not exist "
  << asset << " " << asset2;
  return tecINTERNAL;
+ // LCOV_EXCL_STOP
  }
 
  auto const ammAccountID = (*ammSle)[sfAccount];
  auto sleAMMRoot = sb.peek(keylet::account(ammAccountID));
  if (!sleAMMRoot)
  {
+ // LCOV_EXCL_START
  JLOG(j.error()) << "deleteAMMAccount: AMM account does not exist "
  << to_string(ammAccountID);
  return tecINTERNAL;
+ // LCOV_EXCL_STOP
  }
 
  if (auto const ter =
@@ -266,14 +281,18 @@ deleteAMMAccount(
  if (!sb.dirRemove(
  ownerDirKeylet, (*ammSle)[sfOwnerNode], ammSle->key(), false))
  {
+ // LCOV_EXCL_START
  JLOG(j.error()) << "deleteAMMAccount: failed to remove dir link";
  return tecINTERNAL;
+ // LCOV_EXCL_STOP
  }
  if (sb.exists(ownerDirKeylet) && !sb.emptyDirDelete(ownerDirKeylet))
  {
+ // LCOV_EXCL_START
  JLOG(j.error()) << "deleteAMMAccount: cannot delete root dir node of "
  << toBase58(ammAccountID);
  return tecINTERNAL;
+ // LCOV_EXCL_STOP
  }
 
  sb.erase(ammSle);
@@ -322,11 +341,11 @@ initializeFeeAuctionVote(
  if (tfee != 0)
  ammSle->setFieldU16(sfTradingFee, tfee);
  else if (ammSle->isFieldPresent(sfTradingFee))
- ammSle->makeFieldAbsent(sfTradingFee);
+ ammSle->makeFieldAbsent(sfTradingFee); // LCOV_EXCL_LINE
  if (auto const dfee = tfee / AUCTION_SLOT_DISCOUNTED_FEE_FRACTION)
  auctionSlot.setFieldU16(sfDiscountedFee, dfee);
  else if (auctionSlot.isFieldPresent(sfDiscountedFee))
- auctionSlot.makeFieldAbsent(sfDiscountedFee);
+ auctionSlot.makeFieldAbsent(sfDiscountedFee); // LCOV_EXCL_LINE
 }
 
 } // namespace ripple

src/ripple/app/paths/AMMOffer.h

@@ -74,9 +74,6 @@ class AMMOffer
  Issue const&
  issueIn() const;
 
- Issue const&
- issueOut() const;
-
  AccountID const&
  owner() const;
 

src/ripple/app/paths/impl/AMMOffer.cpp

@@ -44,13 +44,6 @@ AMMOffer<TIn, TOut>::issueIn() const
  return ammLiquidity_.issueIn();
 }
 
-template <typename TIn, typename TOut>
-Issue const&
-AMMOffer<TIn, TOut>::issueOut() const
-{
- return ammLiquidity_.issueOut();
-}
-
 template <typename TIn, typename TOut>
 AccountID const&
 AMMOffer<TIn, TOut>::owner() const

src/ripple/app/tx/impl/AMMDeposit.cpp

@@ -186,16 +186,18 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
  FreezeHandling::fhIGNORE_FREEZE,
  ctx.j);
  if (!expected)
- return expected.error();
+ return expected.error(); // LCOV_EXCL_LINE
  auto const [amountBalance, amount2Balance, lptAMMBalance] = *expected;
  if (ctx.tx.getFlags() & tfTwoAssetIfEmpty)
  {
  if (lptAMMBalance != beast::zero)
  return tecAMM_NOT_EMPTY;
  if (amountBalance != beast::zero || amount2Balance != beast::zero)
  {
+ // LCOV_EXCL_START
  JLOG(ctx.j.debug()) << "AMM Deposit: tokens balance is not zero.";
  return tecINTERNAL;
+ // LCOV_EXCL_STOP
  }
  }
  else
@@ -205,9 +207,11 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
  if (amountBalance <= beast::zero || amount2Balance <= beast::zero ||
  lptAMMBalance < beast::zero)
  {
+ // LCOV_EXCL_START
  JLOG(ctx.j.debug())
  << "AMM Deposit: reserves or tokens balance is zero.";
  return tecINTERNAL;
+ // LCOV_EXCL_STOP
  }
  }
 
@@ -254,10 +258,12 @@ AMMDeposit::preclaim(PreclaimContext const& ctx)
  if (auto const ter =
  requireAuth(ctx.view, amount->issue(), accountID))
  {
+ // LCOV_EXCL_START
  JLOG(ctx.j.debug())
  << "AMM Deposit: account is not authorized, "
  << amount->issue();
  return ter;
+ // LCOV_EXCL_STOP
  }
  // AMM account or currency frozen
  if (isFrozen(ctx.view, ammAccountID, amount->issue()))
@@ -339,7 +345,7 @@ AMMDeposit::applyGuts(Sandbox& sb)
  auto const lpTokensDeposit = ctx_.tx[~sfLPTokenOut];
  auto ammSle = sb.peek(keylet::amm(ctx_.tx[sfAsset], ctx_.tx[sfAsset2]));
  if (!ammSle)
- return {tecINTERNAL, false};
+ return {tecINTERNAL, false}; // LCOV_EXCL_LINE
  auto const ammAccountID = (*ammSle)[sfAccount];
 
  auto const expected = ammHolds(
@@ -350,7 +356,7 @@ AMMDeposit::applyGuts(Sandbox& sb)
  FreezeHandling::fhZERO_IF_FROZEN,
  ctx_.journal);
  if (!expected)
- return {expected.error(), false};
+ return {expected.error(), false}; // LCOV_EXCL_LINE
  auto const [amountBalance, amount2Balance, lptAMMBalance] = *expected;
  auto const tfee = (lptAMMBalance == beast::zero)
  ? ctx_.tx[~sfTradingFee].value_or(0)
@@ -421,8 +427,10 @@ AMMDeposit::applyGuts(Sandbox& sb)
  lptAMMBalance.issue(),
  tfee);
  // should not happen.
+ // LCOV_EXCL_START
  JLOG(j_.error()) << "AMM Deposit: invalid options.";
  return std::make_pair(tecINTERNAL, STAmount{});
+ // LCOV_EXCL_STOP
  }();
 
  if (result == tesSUCCESS)
@@ -621,10 +629,12 @@ AMMDeposit::equalDepositTokens(
  }
  catch (std::exception const& e)
  {
+ // LCOV_EXCL_START
  JLOG(j_.error()) << "AMMDeposit::equalDepositTokens exception "
  << e.what();
+ return {tecINTERNAL, STAmount{}};
+ // LCOV_EXCL_STOP
  }
- return {tecINTERNAL, STAmount{}};
 }
 
 /** Proportional deposit of pool assets with the constraints on the maximum