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

Added negative tests #55

Merged
merged 2 commits into from
Dec 20, 2024
Merged

Added negative tests #55

merged 2 commits into from
Dec 20, 2024

Conversation

traiansf
Copy link
Member

No description provided.

@traiansf traiansf requested a review from sskeirik December 20, 2024 19:05
@traiansf traiansf self-assigned this Dec 20, 2024
Copy link

@sskeirik sskeirik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks! See attached minor comments.

Comment on lines 110 to 112
except ContractLogicError as e:
print(f'Contract logic error: {e.message}', file=sys.stderr)
sys.exit(1)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right logic, but I have a slight preference to put it in the if/then/else statement above since it shares the same logic by:

  1. adding the ContractLogicError to the tuple of caught errors and;
  2. adding a new case to the if/then/else statement for printing its message

assert_eq "1000" "$supply" "Total Supply"

echo -e -n "\nTransfer too much test (should fail): "
erc20_transfer $k2 $contract $a3 $(to_hex 600) || true # this should fail
Copy link

@sskeirik sskeirik Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to fail here if the call passes (my understanding is the call will always be considering passing with || true appended) --- though it should not be possible for the call to pass here and for the remaining calls to still pass.

The same holds true for the other negative case.

@traiansf traiansf merged commit 072367b into master Dec 20, 2024
2 checks passed
@traiansf traiansf deleted the negative_tests branch December 20, 2024 21:44
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.

2 participants