Skip to content

Conversation

mvadari
Copy link
Collaborator

@mvadari mvadari commented Aug 28, 2025

High Level Overview of Change

This PR, as the title says, adds more comprehensive tests for the FeeVote module.

Previously, the tests only checked the basics, and not the more comprehensive flows in that class.

There is no source code change in this PR.

Context of Change

In preparation for #5600 - instead of including these (not-feature-specific) tests in that PR, I'd prefer to just add the Smart Escrow-specific tests there, making it easier to review (and decreasing the diff a bit).

Type of Change

  • Tests (you added tests for code that already exists, or your new feature included in this PR)

Test Plan

This PR is only tests. The tests pass in CI.

@mvadari mvadari requested a review from ximinez August 28, 2025 17:19
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.9%. Comparing base (cfd26f4) to head (de9a1c6).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5746   +/-   ##
=======================================
  Coverage     78.9%   78.9%           
=======================================
  Files          816     816           
  Lines        72246   72246           
  Branches      8418    8418           
=======================================
+ Hits         56993   56994    +1     
+ Misses       15253   15252    -1     

see 7 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mvadari mvadari mentioned this pull request Aug 28, 2025
1 task
@ximinez ximinez requested a review from vvysokikh1 August 29, 2025 15:04
@vvysokikh1
Copy link
Collaborator

I understand that XRPFees was enabled around 1.5 years ago, so the legacy behavior is kind of supported, but I'm not sure adding legacy behavior tests at this point makes any sense

@mvadari
Copy link
Collaborator Author

mvadari commented Sep 15, 2025

I understand that XRPFees was enabled around 1.5 years ago, so the legacy behavior is kind of supported, but I'm not sure adding legacy behavior tests at this point makes any sense

I can remove that part if you'd like (it was more for full codecov etc). IMO the more important aspect is the fee voting tests, as fee voting does still need to happen every flag ledger.

@vvysokikh1
Copy link
Collaborator

I understand that XRPFees was enabled around 1.5 years ago, so the legacy behavior is kind of supported, but I'm not sure adding legacy behavior tests at this point makes any sense

I can remove that part if you'd like (it was more for full codecov etc). IMO the more important aspect is the fee voting tests, as fee voting does still need to happen every flag ledger.

I don't have a strong call on this, just a general remark, so this is up to you. Personally I think those are not worth adding at this point.

Copy link
Collaborator

@vvysokikh1 vvysokikh1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@ximinez ximinez removed their request for review September 24, 2025 18:35
@mvadari mvadari added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Sep 26, 2025
@bthomee bthomee enabled auto-merge (squash) September 26, 2025 17:33
@bthomee bthomee merged commit d02c306 into XRPLF:develop Sep 26, 2025
41 of 42 checks passed
legleux pushed a commit to legleux/rippled that referenced this pull request Oct 1, 2025
This change adds more comprehensive tests for the `FeeVote` module, which previously only checked the basics, and not the more comprehensive flows in that class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants