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

#86drpnd2j - Refactor example_tests/test_ico.py to use BoaConstructor #1228

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

jplippi
Copy link
Contributor

@jplippi jplippi commented Mar 26, 2024

Summary or solution description
Refactored example_tests/test_ico.py files to use BoaTestCase in place of BoaTest for unit testing.

@jplippi jplippi requested a review from meevee98 March 26, 2024 04:15
@melanke
Copy link
Contributor

melanke commented Mar 26, 2024

@@ -380,9 +380,9 @@ def transfer_from(originator: UInt160, from_address: UInt160, to_address: UInt16
:raise AssertionError: raised if `from_address` or `to_address` length is not 20 or if `amount` if less than zero.
"""
# the parameters from and to should be 20-byte addresses. If not, this method should throw an exception.
assert len(originator) == 20 and len(from_address) == 20 and len(to_address) == 20
assert len(originator) == 20 and len(from_address) == 20 and len(to_address) == 20, 'invalid account length'

Choose a reason for hiding this comment

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

It would be more helpful to provide separate error messages for each condition in the assertion. This way, if the assertion fails, it will be easier to identify which condition was not met.

# the parameter amount must be greater than or equal to 0. If not, this method should throw an exception.
assert amount >= 0
assert amount >= 0, 'invalid amount'

Choose a reason for hiding this comment

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

Consider checking if amount is an integer before checking if it's greater than or equal to 0. This will prevent potential type errors.

@@ -440,8 +440,8 @@ def approve(originator: UInt160, to_address: UInt160, amount: int) -> bool:
:return: whether the approval was successful
:raise AssertionError: raised if `originator` or `to_address` length is not 20 or if `amount` if less than zero.
"""
assert len(originator) == 20 and len(to_address) == 20
assert amount >= 0
assert len(originator) == 20 and len(to_address) == 20, 'invalid account length'

Choose a reason for hiding this comment

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

Consider splitting the combined assertion into two separate assertions. This will make the code more readable and easier to debug, as you will know exactly which condition failed.

assert len(originator) == 20 and len(to_address) == 20
assert amount >= 0
assert len(originator) == 20 and len(to_address) == 20, 'invalid account length'
assert amount >= 0, 'invalid amount'

Choose a reason for hiding this comment

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

Consider adding more context to the error message. For example, 'invalid amount: amount cannot be negative' would provide more information about why the error occurred.

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 91.954% (+0.005%) from 91.949%
when pulling a144b5a on CU-86drpnd2j
into 7fd1f46 on development.

@meevee98 meevee98 merged commit d15ecaf into development Mar 26, 2024
5 checks passed
@meevee98 meevee98 deleted the CU-86drpnd2j branch March 26, 2024 19:39
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.

4 participants