search

XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.48k stars 1.45k forks source link

Increase test coverage #4971

Closed thejohnfreeman closed 2 months ago

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/4971/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/4971?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) ```diff @@ 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](https://app.codecov.io/gh/XRPLF/rippled/pull/4971?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) | Coverage Δ | | |---|---|---| | [src/ripple/app/misc/impl/AMMUtils.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4971?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fmisc%2Fimpl%2FAMMUtils.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvbWlzYy9pbXBsL0FNTVV0aWxzLmNwcA==) | `99.2% <ø> (+17.5%)` | :arrow_up: | | [src/ripple/app/paths/AMMOffer.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4971?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fpaths%2FAMMOffer.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvcGF0aHMvQU1NT2ZmZXIuaA==) | `83.3% <ø> (ø)` | | | [src/ripple/app/paths/impl/AMMOffer.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4971?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Fpaths%2Fimpl%2FAMMOffer.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvcGF0aHMvaW1wbC9BTU1PZmZlci5jcHA=) | `81.8% <ø> (+2.9%)` | :arrow_up: | | [src/ripple/app/tx/impl/AMMDeposit.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4971?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Ftx%2Fimpl%2FAMMDeposit.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvdHgvaW1wbC9BTU1EZXBvc2l0LmNwcA==) | `95.1% <ø> (+8.2%)` | :arrow_up: | | [src/ripple/app/tx/impl/AMMWithdraw.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4971?src=pr&el=tree&filepath=src%2Fripple%2Fapp%2Ftx%2Fimpl%2FAMMWithdraw.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9hcHAvdHgvaW1wbC9BTU1XaXRoZHJhdy5jcHA=) | `90.4% <ø> (+6.1%)` | :arrow_up: | | [src/ripple/protocol/STIssue.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4971?src=pr&el=tree&filepath=src%2Fripple%2Fprotocol%2FSTIssue.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9wcm90b2NvbC9TVElzc3VlLmg=) | `80.0% <ø> (+5.0%)` | :arrow_up: | | [src/ripple/protocol/impl/STIssue.cpp](https://app.codecov.io/gh/XRPLF/rippled/pull/4971?src=pr&el=tree&filepath=src%2Fripple%2Fprotocol%2Fimpl%2FSTIssue.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-c3JjL3JpcHBsZS9wcm90b2NvbC9pbXBsL1NUSXNzdWUuY3Bw) | `93.5% <ø> (+10.7%)` | :arrow_up: | ... and [10 files with indirect coverage changes](https://app.codecov.io/gh/XRPLF/rippled/pull/4971/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/4971/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/4971?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)
thejohnfreeman commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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.