Skip to content
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

Merged
merged 13 commits into from
Feb 28, 2024

Conversation

cardenaso11
Copy link
Contributor

@cardenaso11 cardenaso11 commented Feb 5, 2024

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


coin_a_amt_sans_protocol_fees >= 1,
coin_b_amt >= 1,
// BUG SSW-201
Copy link
Member

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.

@cardenaso11 cardenaso11 force-pushed the SB-1337/start-test-implementation branch from 9371394 to fceb461 Compare February 28, 2024 06:26
@Quantumplation Quantumplation marked this pull request as ready for review February 28, 2024 18:24
Copy link
Member

@Quantumplation Quantumplation left a 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 different example 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)

@Quantumplation Quantumplation merged commit dc9c994 into main Feb 28, 2024
1 check passed
@Quantumplation Quantumplation deleted the SB-1337/start-test-implementation branch February 28, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants