8000 Fix delegate test silently fail with batch by yinyiqian1 · Pull Request #5476 · XRPLF/rippled · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix delegate test silently fail with batch #5476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 11, 2025

Conversation

yinyiqian1
Copy link
Collaborator

The test to make sure tfInnerBatchTxn won't block delegated transactions would silently fail in Delegate_test.cpp.
This commit removes these cases from Delegate_test.cpp. and adds them to Batch_test.cpp.
This will not silently fail because we will explicitly check batch delegate result.
Moving to Batch_test.cpp to avoid refactoring too many helper functions.

High Level Overview of Change

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

Copy link
codecov bot commented Jun 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.1%. Comparing base (35a40a8) to head (b195003).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5476     +/-   ##
=========================================
- Coverage     79.1%   79.1%   -0.0%     
=========================================
  Files          817     817             
  Lines        71703   71703             
  Branches      8237    8240      +3     
=========================================
- Hits         56721   56711     -10     
- Misses       14982   14992     +10     

see 3 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tesSUCCESS,
batch::outer(gw, seq, batchFee, tfAllOrNothing),
batch::inner(jv1, seq + 1),
// tecNO_DELEGATE_PERMISSION: not authorized by the delegating
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's failing on the first not the second right?

I think using the tfIndependent flag is a better choice for these failing tests.

I realize all my tests used tfAllOrNothing but going forward I think its much easier to understand whats happening if we use tfIndependent and we can visualize the errors easier. So then both these results should be tecNO_DELEGATE_PERMISSION and they should be in the ledger.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator
@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

This is really nice.

@Bronek
Copy link
Collaborator
Bronek commented Jun 10, 2025

Please wait for @dangell7 review before merging.

EDIT: reviewed

@Bronek
Copy link
Collaborator
Bronek commented Jun 10, 2025

@yinyiqian1 is this ready to merge ?

@yinyiqian1
Copy link
Collaborator Author
yinyiqian1 commented Jun 10, 2025

@yinyiqian1 is this ready to merge ?

@Bronek Yes. It is ready.

@bthomee bthomee merged commit ea17abb into XRPLF:develop Jun 11, 2025
26 checks passed
@kennyzlei kennyzlei added this to the 2.5.0 (Q2 2025) milestone Jun 11, 2025
This was referenced Jun 12, 2025
@legleux legleux mentioned this pull request Jun 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0