[WIP] Feat/better lending tests #1062

dummytester123 · 2021-01-14T02:54:35Z

As part of learning rust, going to gradually improve the lending platform's test suite.

… after repayment. Standing on shoulders of giants :)

jstarry · 2021-01-15T01:16:31Z

As part of learning rust, going to gradually improve the lending platform's test suite.

Awesome! FYI the repay test was just expanded here: #1068

…e program each time for more atomic tests, and have each test test a separate piece of the program.

dummytester123 · 2021-01-16T01:10:15Z

Hey @jstarry I feel like I just retread your exact foot steps here! I did not know you were also working on this. I learned a lot doing it though, which feels great. Is there another instruction here I could assist in further extending?

jstarry · 2021-01-16T01:37:33Z

Hey @jstarry I feel like I just retread your exact foot steps here! I did not know you were also working on this. I learned a lot doing it though, which feels great. Is there another instruction here I could assist in further extending?

Haha, you had good intuition, repay was the least tested and deserved another look. I'd recommend the liquidation instruction 👍

dummytester123 · 2021-01-17T20:13:27Z

Roger that. Will give it a whirl. Going to try to add checks on the other mutated fields!

…bligation works

dummytester123 · 2021-01-18T02:59:40Z

token-lending/program/tests/liquidate.rs

@@ -146,7 +182,19 @@ async fn test_success() {
        usdc_liquidated,
        usdc_loan_state.borrowed_liquidity_wads.round_u64()
    );
+    let collateral_liquidated =
+        USDC_LOAN_SOL_COLLATERAL - usdc_loan_state.deposited_collateral_tokens;
+    assert!(collateral_liquidated > 0)


Collateral is a tough one to test exactly because the math to get to it is complex. We're either hard coding it (which is not really a good clear cut test) or replicating the logic (which isn't really testing at all.) For now, I just asserted that it changed. Not great, but better than nothing.

Yeah it's tough. I think we'll want to mock out the dex markets to get nice numbers to work with. Do you want to give that a shot?

Hey @jstarry I couldn't figure out a good way to really stub out the Dex Market here. There is this cool library called https://docs.rs/mockall/0.9.0/mockall/ that could be useful but it didn't really pan out for the job because it's pretty invasive and doesn't seem to work with the lifetime parameters in use with the TradeSimulator, and overall Rust is just not that friendly with stubbing. The other thing that's at issue is that the liquidator is instantiating the TradeSimulator on the go vs being passed one in, and the mockall library relies heavily on you mocking the object before passing it into something else. I'm not quite sure how to get around that. Do you have any good ideas here? I don't want to spin my wheels on this too much more, as it seems like a potential deep well!

I was imagining that we could fill the dex market account data with specific orders dynamically. This would require deeper knowledge of how serum's data structures work and then serialize that data into the dex market order book accounts to seed the program tests.

token-lending/program/tests/liquidate.rs

token-lending/program/tests/helpers/mod.rs

dummytester123 · 2021-01-18T03:02:23Z

@jstarry Still very much a WIP, but I spent a good chunk of today experimenting with liquidate and I refactored liquidate.rs (the test file) a lot, and added one new test around the healthy obligation check. I was wondering if you would not mind checking out my syntax, and style, and letting me know if I'm on the right track here. I think a good approach would be to write tests around the validation checks first as this is really important and low hanging fruit, healthy obligation being my first attempt. Thanks! I left line comments to give context to my thought process.

jstarry

Thanks for the contribution, here's some initial feedback!

token-lending/program/tests/helpers/mod.rs

token-lending/program/tests/repay.rs

jstarry · 2021-01-18T15:25:14Z

token-lending/program/tests/repay.rs

-
-    const OBLIGATION_LOAN: u64 = 1;
-    const OBLIGATION_COLLATERAL: u64 = 500;
+    test.set_bpf_compute_max_units(NUMBER_OF_TESTS * 80_000);


I don't see NUMBER_OF_TESTS used elsewhere, what is its purpose here?

I noticed that this becomes a global counter even if you reset test multiple times - it's tied per-file I think, or potentially per run. So I was using it to scale up the number of maximum instructions to the number of tests you had in the file. Trying to figure out a better way to reset this each time you call "setup."

The max units param restricts the number of compute units allowed for each transaction. It shouldn't be accumulated by number of tests. You should be able to set a new value per test if you want

Hmm ok I will look into this

So I did look into this, and indeed, even though each ProgramTest::new ostensibly makes a new ProgramTest object, it actually stores in memory by spl_token_lending::id() the program instruction count, so if you create it again, it doesn't reset the counter. So if I remove the number_of_tests variable, ie set it to 80,000 and just remake it each time, I run out of instructions and the tests die. However, if I just run a single test, the test is fine, because 80k for one is alright. I think this hasn't been a problem for you before because each of your files only has one test @jstarry

Oh really? Nice find, that's a bug in the program test framework then. Do you mind writing an issue over in the main monorepo project describing the problem?

jstarry · 2021-01-18T15:30:28Z

token-lending/program/tests/liquidate.rs

@@ -146,7 +182,19 @@ async fn test_success() {
        usdc_liquidated,
        usdc_loan_state.borrowed_liquidity_wads.round_u64()
    );
+    let collateral_liquidated =
+        USDC_LOAN_SOL_COLLATERAL - usdc_loan_state.deposited_collateral_tokens;
+    assert!(collateral_liquidated > 0)


Yeah it's tough. I think we'll want to mock out the dex markets to get nice numbers to work with. Do you want to give that a shot?

token-lending/program/tests/liquidate.rs

… a bit more robust

dummytester123 · 2021-01-21T03:32:27Z

Alright, some minor updates tonight - took into account your feedback @jstarry , and did a bunch of refactoring. Next up, going to try to stub out that Dex market! Thanks for all your time so far on this.

jstarry · 2021-01-21T12:21:09Z

Alright, some minor updates tonight - took into account your feedback @jstarry , and did a bunch of refactoring. Next up, going to try to stub out that Dex market! Thanks for all your time so far on this.

No problem, thank you!

…ts up the program each time for more atomic tests, and have each test test a separate piece of the program." This reverts commit 997d83a.

… of loan after repayment. Standing on shoulders of giants :)" This reverts commit 31c2957.

dummytester123 · 2021-01-22T03:06:12Z

@jstarry I removed the repay file with a couple git reverts, answered your query about NUMBER_OF_TESTS, and did a follow up on stubbing the dex market (turns out it's tougher than anticipated).

jordaaash · 2021-04-29T02:05:55Z

Closing after discussion with author, #1441 changes most of this.

Tiny little first attempt at learning the repo: Check balance of loan…

31c2957

… after repayment. Standing on shoulders of giants :)

dummytester123 marked this pull request as draft January 14, 2021 02:54

dummytester123 force-pushed the feat/better-lending-tests branch from ba32ad7 to 31c2957 Compare January 14, 2021 02:58

Refactor repay tests a bit to allow for a test helper that sets up th…

997d83a

…e program each time for more atomic tests, and have each test test a separate piece of the program.

Refactor liquidate tests a bit and add a new one that tests healthy o…

ea0ff70

…bligation works

dummytester123 commented Jan 18, 2021

View reviewed changes

token-lending/program/tests/liquidate.rs Show resolved Hide resolved

dummytester123 commented Jan 18, 2021

View reviewed changes

token-lending/program/tests/liquidate.rs Show resolved Hide resolved

dummytester123 commented Jan 18, 2021

View reviewed changes

token-lending/program/tests/liquidate.rs Outdated Show resolved Hide resolved

dummytester123 commented Jan 18, 2021

View reviewed changes

token-lending/program/tests/helpers/mod.rs Show resolved Hide resolved

jstarry reviewed Jan 18, 2021

View reviewed changes

dummytester123 added 2 commits January 20, 2021 20:46

refactor liquidate tests based on Justin Starry's feedback to make it…

1dac731

… a bit more robust

Switch to unwrapping to transaction error

5c361d6

dummytester123 added 3 commits January 21, 2021 20:53

Revert "Refactor repay tests a bit to allow for a test helper that se…

e021e71

…ts up the program each time for more atomic tests, and have each test test a separate piece of the program." This reverts commit 997d83a.

Revert "Tiny little first attempt at learning the repo: Check balance…

0312b58

… of loan after repayment. Standing on shoulders of giants :)" This reverts commit 31c2957.

Remove print statement that's unneeded

8206abf

Merge branch 'master' into feat/better-lending-tests

9595232

jordaaash closed this Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Feat/better lending tests #1062

[WIP] Feat/better lending tests #1062

dummytester123 commented Jan 14, 2021

jstarry commented Jan 15, 2021

dummytester123 commented Jan 16, 2021

jstarry commented Jan 16, 2021

dummytester123 commented Jan 17, 2021

dummytester123 Jan 18, 2021

jstarry Jan 18, 2021

dummytester123 Jan 22, 2021

jstarry Jan 22, 2021

dummytester123 commented Jan 18, 2021 •

edited

Loading

jstarry left a comment

jstarry Jan 18, 2021

dummytester123 Jan 19, 2021

jstarry Jan 19, 2021

dummytester123 Jan 21, 2021

dummytester123 Jan 22, 2021

jstarry Jan 22, 2021

jstarry Jan 18, 2021

dummytester123 commented Jan 21, 2021

jstarry commented Jan 21, 2021

dummytester123 commented Jan 22, 2021

jordaaash commented Apr 29, 2021

[WIP] Feat/better lending tests #1062

[WIP] Feat/better lending tests #1062

Conversation

dummytester123 commented Jan 14, 2021

jstarry commented Jan 15, 2021

dummytester123 commented Jan 16, 2021

jstarry commented Jan 16, 2021

dummytester123 commented Jan 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dummytester123 commented Jan 18, 2021 • edited Loading

jstarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dummytester123 commented Jan 21, 2021

jstarry commented Jan 21, 2021

dummytester123 commented Jan 22, 2021

jordaaash commented Apr 29, 2021

dummytester123 commented Jan 18, 2021 •

edited

Loading