Increase test coverage #4971

thejohnfreeman · 2024-03-29T19:29:12Z

No description provided.

gregtatcam

Thanks for extending AMM code coverage!
I left a few minor comments.
I still need to go over LCOV_EXCL to check whether it can be covered or not. I wanted to provide the feedback on the unit-tests sooner.

src/test/app/AMMWithdraw_test.cpp

src/test/app/AMM_test.cpp

src/test/jtx/AMM.h

gregtatcam

Added LCOV_EXCL looks correct.

codecov-commenter · 2024-04-02T19:19:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.9%. Comparing base (aae4383) to head (20e69b2).

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #4971     +/-   ##
=========================================
+ Coverage     70.8%   70.9%   +0.1%     
=========================================
  Files          796     796             
  Lines        66785   66727     -58     
  Branches     11034   10978     -56     
=========================================
+ Hits         47301   47341     +40     
+ Misses       19484   19386     -98

Files	Coverage Δ
src/ripple/app/misc/impl/AMMUtils.cpp	`99.2% <ø> (+17.5%)`	⬆️
src/ripple/app/paths/AMMOffer.h	`83.3% <ø> (ø)`
src/ripple/app/paths/impl/AMMOffer.cpp	`81.8% <ø> (+2.9%)`	⬆️
src/ripple/app/tx/impl/AMMDeposit.cpp	`95.1% <ø> (+8.2%)`	⬆️
src/ripple/app/tx/impl/AMMWithdraw.cpp	`90.4% <ø> (+6.1%)`	⬆️
src/ripple/protocol/STIssue.h	`80.0% <ø> (+5.0%)`	⬆️
src/ripple/protocol/impl/STIssue.cpp	`93.5% <ø> (+10.7%)`	⬆️

... and 10 files with indirect coverage changes

thejohnfreeman · 2024-04-04T22:21:57Z

@gregtatcam can you help me try to convert the last tests, in 43a251c, to use BidArg? I don't understand why my attempt fails.

gregtatcam · 2024-04-04T22:41:13Z

@gregtatcam can you help me try to convert the last tests, in 43a251c, to use BidArg? I don't understand why my attempt fails.

Will do

gregtatcam · 2024-04-04T22:56:51Z

@gregtatcam can you help me try to convert the last tests, in 43a251c, to use BidArg? I don't understand why my attempt fails.

@thejohnfreeman submit is missing in AMM::bid(BidArg const& arg). It just returns Json::Value.

thejohnfreeman · 2024-04-04T23:01:29Z

@gregtatcam Right, that's why the return value is passed to env, e.g. env(bid({ ... }));, to apply the transaction. I think this more closely follows the original design of these tests: build a JSON transaction object, then apply it like env(tx), with optional extras like msig, seq, and ter. This way, tests can intercept the JSON transaction before it is applied, to "disturb" it in some way and test that the disturbance leads to an expected error. If bid(...) applies the transaction with no chance to intercept, then that opportunity is lost.

gregtatcam · 2024-04-04T23:25:46Z

@gregtatcam Right, that's why the return value is passed to env, e.g. env(bid({ ... }));, to apply the transaction. I think this more closely follows the original design of these tests: build a JSON transaction object, then apply it like env(tx), with optional extras like msig, seq, and ter. This way, tests can intercept the JSON transaction before it is applied, to "disturb" it in some way and test that the disturbance leads to an expected error. If bid(...) applies the transaction with no chance to intercept, then that opportunity is lost.

Right, sorry. But then all tests would have to be changed. I'm doing a few things in submit as well.

gregtatcam · 2024-04-04T23:30:39Z

@gregtatcam Right, that's why the return value is passed to env, e.g. env(bid({ ... }));, to apply the transaction. I think this more closely follows the original design of these tests: build a JSON transaction object, then apply it like env(tx), with optional extras like msig, seq, and ter. This way, tests can intercept the JSON transaction before it is applied, to "disturb" it in some way and test that the disturbance leads to an expected error. If bid(...) applies the transaction with no chance to intercept, then that opportunity is lost.

Right, sorry. But then all tests would have to be changed. I'm doing a few things in submit as well.

submit calls env.close();. So if you add this after each bid then it'll work. But then again, to make this consistent deposit,withdraw,vote would have to change too. This is pretty substantial change.

thejohnfreeman · 2024-04-05T18:42:09Z

@gregtatcam I only changed the calls to bid() (not deposit(), withdraw(), etc.) because I had to touch them anyway to restore BidArg, and as a proof-of-concept. I don't plan to move on to the others, but I think we should take away some lessons on how to design these tests going forward. The revision of existing tests can be left for future work. "Good first issue" kind of thing.

gregtatcam · 2024-04-05T18:44:25Z

@gregtatcam I only changed the calls to bid() (not deposit(), withdraw(), etc.) because I had to touch them anyway to restore BidArg, and as a proof-of-concept. I don't plan to move on to the others, but I think we should take away some lessons on how to design these tests going forward. The revision of existing tests can be left for future work. "Good first issue" kind of thing.

Thanks. Will review today or Monday.

src/test/app/AMM_test.cpp

gregtatcam

Overall looks good.
Remaining areas to address:

Move AMMWithdraw_test to AMM_test. It would be great overall to be able to run tests individually like AMMWitrhdraw, AMMDeposit, etc.; but not in this PR.
Updated bid method changes the default behavior. I.e., env.close() is generally called after every bid but the updated tests don't always call close. Should it be added?
Can deposit non-frozen token comment should be updated.
There are two suggestions to consider for the comments update.

src/test/app/AMM_test.cpp

Bronek

Note, if we merged https://app.codecov.io/gh/XRPLF/rippled/pull/4977 into develop before this PR, then codecov statistics would be relative to the changes (we would have merged) in #4977 , and it would be quite visible that this PR has increased overall coverage from 70.8% to 70.9%

Bronek · 2024-04-12T18:56:17Z

src/ripple/protocol/STIssue.h

@@ -56,9 +56,6 @@ class STIssue final : public STBase, CountedObject<STIssue>
    SerializedTypeID
    getSType() const override;

-    std::string
-    getText() const override;
-
    Json::Value getJson(JsonOptions) const override;


I love how this PR also contains the removal of dead code 🥇

gregtatcam

LGTM

seelabs

👍

seelabs · 2024-04-17T22:24:36Z

src/test/jtx/Env.h

+        editing the JSON.
+    */
+    std::shared_ptr<STTx const>
+    ust(JTx const& jt);


Consider giving this a more descriptive name

@mtrippled please suggest a name. I don't know what this should be named, or what "ust" stands for.

Unsanitized serialized transaction?

@ximinez I think John wanted to update the name of this function before the merge 😅

Woops. @thejohnfreeman, if you (or someone else) wants to open a "trivial" PR with the name change, I'll approve and merge it.

seelabs · 2024-04-17T22:35:39Z

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


I looked at the exclusions, and I'm OK with keeping them. I might feel differently if there were an excessive number of them, but there are relatively few and seem reasonably justified. I don't think we're in danger if we keep them.

I'm guessing we'll do another PR in the future with more exclusions. We may want to revisit some of these exclusions in that PR, but this is OK for now and also not the end of the world if we don't revisit in the future PR.

--------- Co-authored-by: Howard Hinnant <[email protected]> Co-authored-by: Mark Travis <[email protected]> Co-authored-by: Bronek Kozicki <[email protected]> Co-authored-by: Mayukha Vadari <[email protected]> Co-authored-by: Chenna Keshava <[email protected]>

thejohnfreeman and others added 9 commits March 28, 2024 11:57

Add coverage

c7e0da5

Suppress warning: unused variable 'index'

c210ad4

Remove unused code.

779b381

Tests for AMMBid::preflight()

c688be6

Test comparison of issued assets to cover STIssue.h.

f6b1f68

Increase unit tests coverage

a8c0884

update test coverage for AMMDeposit

48ce399

remove unreachable lines from codecov

faad55f

Fix formatting

8975ddf

thejohnfreeman requested a review from gregtatcam March 29, 2024 19:29

gregtatcam requested changes Apr 1, 2024

View reviewed changes

gregtatcam reviewed Apr 1, 2024

View reviewed changes

Bronek and others added 6 commits April 2, 2024 14:33

Disable wandalen/wretry for codecov upload

7f151fa

AMMBid unit test: burn all LP tokens

87c8acd

Edit comments

51eaed5

Consolidate disabled amendment tests

61d007e

Convert most bid tests to use BidArg

a504e86

Try to convert last bid tests

43a251c

Format code

2088454

thejohnfreeman added 4 commits April 5, 2024 12:08

Finish bid tests

242b7ec

Merge branch 'ckeshava/burnTest'

b8fdf58

Merge burn tests and eliminate non-BidArg overload

1fa1581

Merge upstream 'develop'

e13b7e5

Fix formatting

c6128d1

gregtatcam reviewed Apr 7, 2024

View reviewed changes