-
Notifications
You must be signed in to change notification settings - Fork 3
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
Sb 1337/Start test implementation #51
Conversation
validators/pool.ak
Outdated
|
||
coin_a_amt_sans_protocol_fees >= 1, | ||
coin_b_amt >= 1, | ||
// BUG SSW-201 |
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.
Please revert this change, and make it it's own standalone PR with a test;
Each finding should be alone in it's own PR, whereas this PR is just for broader testing of the whole protocol.
Also, no need to make reference to the finding in the comment; just explain why we're doing what we're doing.
Co-authored-by: card <[email protected]>
…s just to have a working macos version of aiken since cargo was giving trouble
9371394
to
fceb461
Compare
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.
I think this is now in a good state to merge, and we can continue to refine it in follow ups.
Some things I'd like to see:
- pull some of the data-setup stuff into
lib/tests
; the idea is to provide differentexample
functions for different types of sample test data, so that the tests themselves can be pretty minimal and readable. as it is right now, i have very little idea what these are testing, because the code is so complex. - Tests for each feature, at a minimum
- Negative tests, that confirm that you can't do something
- some kind of structure to how we organize the tests, so we can correlate it to the test cases spreadsheet we've been putting together (or, if you have an alternative proposal, open to that)
Tests should be fine but there's a bit of stuff related to Elaine's nix/macos environment for Aiken development that should be removed before merge