Add RPC error checking support to unit tests #4987
Merged
Conversation
This file contains 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
ximinez
added
the
Perf impact not expected
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4987 +/- ##
=========================================
- Coverage 71.0% 71.0% -0.0%
=========================================
Files 796 796
Lines 66727 66727
Branches 10978 10973 -5
=========================================
- Hits 47347 47346 -1
- Misses 19380 19381 +1 see 2 files with indirect coverage changes |
ckeshava
reviewed
Apr 12, 2024
src/test/jtx/Env_test.cpp
Outdated
| @@ -748,8 +748,12 @@ class Env_test : public beast::unit_test::suite | |||
| params[jss::fee_mult_max] = 1; | |||
| params[jss::fee_div_max] = 2; | |||
| // RPC errors result in telENV_RPC_FAILED | |||
There was a problem hiding this comment.
I feel this comment is more appropriate in src/test/jtx/rpc.h. Specifically, in the operator() of the rpc class.
ckeshava
reviewed
Apr 12, 2024
ckeshava
approved these changes
Apr 12, 2024
Co-authored-by: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com>
scottschurr
reviewed
Apr 17, 2024
There was a problem hiding this comment.
This looks like a good change and the implementation is pretty clever. I spotted a few changes that might be improvements, but feel free to push back on any of them. I put all of my suggestions into a commit that you can see here: https://github.com/scottschurr/rippled/commits/ed-jtx_rpc/ You probably don't want to take all of my suggestions, but the commit will at least show you what I was thinking.
* Make ter and rpcCode optional in ParsedResult * Remove didApply from ParsedResult
* upstream/develop: fix: Remove redundant STAmount conversion in test (4996) fix: resolve database deadlock: (4989) test: verify the rounding behavior of equal-asset AMM deposits (4982) test: Add tests to raise coverage of AMM (4971) chore: Improve codecov coverage reporting (4977) test: Unit test for AMM offer overflow (4986) fix amendment to add `PreviousTxnID`/`PreviousTxnLgrSequence` (4751)
* upstream/develop: Set version to 2.2.0-b3
scottschurr
approved these changes
Apr 22, 2024
|
Proposed commit message: |
ximinez
added
the
Passed
legleux
added a commit
to legleux/rippled
that referenced
this pull request
Apr 26, 2024
* Remove unused files
* Remove packaging scripts
* Consolidate external libraries
* Simplify protobuf generation
* Rename .hpp to .h
* Format formerly .hpp files
* Rewrite includes
$ find src/ripple/ src/test/ -type f -exec sed -i 's:include\s*["<]ripple/\(.*\)\.h\(pp\)\?[">]:include <ripple/\1.h>:' {} +
* Fix source lists
* Add markers around source lists
* Address compiler warnings
* Ignore more commits
* test: Add RPC error checking support to unit tests (XRPLF#4987)
* chore: fix typos (XRPLF#4958)
* file
* new commit
* file sucks
---------
Co-authored-by: John Freeman <jfreeman08@gmail.com>
Co-authored-by: Pretty Printer <cpp@ripple.com>
Co-authored-by: Ed Hennis <ed@ripple.com>
Co-authored-by: Snoppy <michaleli@foxmail.com>
sophiax851
pushed a commit
to sophiax851/rippled
that referenced
this pull request
Jun 12, 2024
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
Adds RPC error verification functionality to unit tests. There are two ways to specify the expected result:
errorstring, and an optionalerror_exceptiondetail message. If the detail message is not specified, it will not be checked.These checks are not comprehensive (for example, nothing checks the optional
error_whatfield), but I think they are sufficient for any expected test case results.Context of Change
PR #4887 introduced a new
TERresult code to indicate that an RPC request failed during transaction processing, but it does not indicate which RPC failure occurred. Inspired by a discussion on that PR, I added the ability to explicitly check the RPC result.(Props to @ckeshava for the pre-review.)
Type of Change
API Impact
None.
Test Plan
There should be no observable external effects for this change. This is a developer tool to help write more and better unit tests.
Future Tasks
Now that we can distinguish RPC failures, it might be useful to add more test cases that explicitly test the various combinations of failures. (Thanks to @ckeshava again for the suggestion.]