Conversation
| // Note: All these tests are quite long-running so I decided to create larger tests (e.g. testing add and remove | ||
| // liquidity happy path just in 1 test case) |
There was a problem hiding this comment.
How long running? Given they run in parallel it's not the end of the world to have long tests, I'm not sure we should take this path so early.
There was a problem hiding this comment.
It's not too bad but it also makes the tests quite boilerplatey. I would say the result is quite ok but can create a separate test case only for adding liquidity if you would prefer.
benesjan
force-pushed
the
08-19-test_amm_noir_tests
branch
2 times, most recently
from
5aaa202 to
ae80a49
Compare
nventuro
left a comment
nventuro
left a comment
There was a problem hiding this comment.
These look much better, thanks. It's still a bit annoying to write these, we'll need to iterate on the style a bit more. Perhaps a struct with scenario config, idk. The fact that we can't return the contract types from functions or put them in structs is very annoying.
| // Note: Ideally this test would be split into 2 separate test cases - one for order creation and one for order | ||
| // fulfillment, with the second test running on the state from the first test. Since this feature is not currently | ||
| // supported, combining them into a single comprehensive test is the best approach. |
There was a problem hiding this comment.
What do you mean by this? What feature?
There was a problem hiding this comment.
I was thinking that it would be cool if you could run test case on state of a previous test case. Since here re-running the add liquidity for the test that would test liquidity removal takes what feels like a lot of resources.
Can nuke the comment if you prefer as I don't care.
Fixes #10289 Just like my [other recent PR](#16451) decided to add TXE tests for AMM. ## On using AI here The only thing I did manually here is to merge a few test cases because they were too long-running as setting up the initial state is costly. I also told it to not have precise ratio in the second call to `add_liquidity` to test the refund. Other than this AI pretty much managed to nail it. ### Update on AI Code quality was not great and had to redo quite a lot of stuff. That could be improved though if we had a proper reference (plenty of the style issues were present in the rest of our codebase).
AztecBot
force-pushed
the
08-19-test_amm_noir_tests
branch
from
96e84c8 to
077fdb4
Compare
3 participants
Fixes #10289
Just like my other recent PR decided to add TXE tests for AMM.
On using AI here
The only thing I did manually here is to merge a few test cases because they were too long-running as setting up the initial state is costly. I also told it to not have precise ratio in the second call to
add_liquidityto test the refund. Other than this AI pretty much managed to nail it.Update on AI
Code quality was not great and had to redo quite a lot of stuff. That could be improved though if we had a proper reference (plenty of the style issues were present in the rest of our codebase).