refactor: V3 swap integration test with persistence #751
Conversation
| client: PublicWalletClient & TestActions; | ||
| snapshotPreApprove: Hex; | ||
| }> { | ||
| const fork = await startFork(test.anvilNetwork, jobId); |
There was a problem hiding this comment.
I think this might be where the current issues come from - at least locally it seems like the jobId isn't unique for each test. So we have multiple tests running in parallel reseting/altering state on the same fork which causes issues with nonces, etc. I think the same might also be happening in a single test file where different tests run in parallel and reset to snapshot. Using sequential should stop that.
brunoguerios
left a comment
brunoguerios
left a comment
There was a problem hiding this comment.
Overall I like the idea of relying on static pre-generated test data 👍
Old test structure is taking forever to run - is that because we're relying in older blockNumbers?
I think there's room to organize things a bit so it's a bit easier to follow, because in order to achieve the new structure a lot complexity was added 😅
Hopefully while refactoring tests for add/remove liquidity it will be easier to identify what's being reused and should be moved to some utils file/folder?
Something to consider is the approach used in balancer-maths where we separate testData generation and consumption in a more clear way.
| outputTest?: { | ||
| testExactOutAmount: boolean; | ||
| percentage: number; | ||
| }; | ||
| inputTest?: { | ||
| testExactInAmount: boolean; | ||
| percentage: number; | ||
| }; |
There was a problem hiding this comment.
If I understood correctly, this is for allowing to config a tolerance on the test result, right?
How about we replace all this with something like percentageTolerance or something like that?
Being input or output is already specified on swapKind and being exact is a special case where percentageTolerance is zero, right?
brunoguerios
left a comment
brunoguerios
left a comment
There was a problem hiding this comment.
This does look a lot better organized 👍
I'd say the improved efficiency should compensate for the increased complexity.
Hopefully this will finally solve our recurring issues with CI 🤞
| ); | ||
| } | ||
| if ( | ||
| data.token.wrapped !== undefined && |
There was a problem hiding this comment.
This will likely need to be refactored after merging main back to this working branch since wrapped was removed from Token
Refactors V3 swap integration tests to use persisted test data for faster runs.
Changes:
Integration tests for V3 swaps covering:
Test data file (test/entities/swaps/v3/swapTestData.json):
Benefits