-
Notifications
You must be signed in to change notification settings - Fork 0
Solana: Settler tests refactor #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: solana/settler
Are you sure you want to change the base?
Conversation
…r-proposal-lifecycle
… plus other suggestions
…r-proposal-lifecycle
PedroAraoz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comments
- If we are going to always do
&[EntityType::Solver as u8 + 1]when using the controller from the settler, I would just change the controller so the enum starts at 0. - Follow the testing convention we use, you can use the EVM Settler as reference
- Extract repeated code into functions
- Always use helper functions, for example when creating the intent it is sometimes done createTestIntent but not always
Helpers.ts
Leaving this here because GH does not allow commenting on files not changed by the PR
- I would split it into multiple files
- I would split createTestIntent into one function that creates the data (like we use in the backend repo) and maybe another that sends the ix and returns the hash
|
Feedback about your earlier message @PedroAraoz:
Granted. Changed
Modified tests to follow that convention better, still unsure if they are properly formatted
Did this with everything I could, let me know if you see anything I missed
Tried to fix that as well, again let me know if I missed anything
I split this into several files now, grouping functionality together
Makes sense, done. |
|
NOTE: The |
Changes Settler unit test pattern to use
context()as requested in previous PRs (comes from #50).