-
Notifications
You must be signed in to change notification settings - Fork 115
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
chain: refactor transaction tests #1748
base: main
Are you sure you want to change the base?
Conversation
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.
Nice! Thanks for refactoring this to better share setup code.
Left one comment to use a setup helper function rather than use the testify Suite
library
chain/transaction_test.go
Outdated
func (*action2) GetTypeID() uint8 { | ||
return 222 | ||
func TestTransactionTestSuite(t *testing.T) { | ||
suite.Run(t, new(TransactionTestSuite)) |
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'm not sure testify suite adds much value to the tests beyond what we can do with a single helper function at the start of each test and would prefer not to introduce it here as the only test where we start using this tool.
Could we instead share test setup code via a single setup helper function that returns the fields that have currently been added to TransactionTestSuite
?
Changes have been applied 👍 |
Closes #1702 and refactors transaction tests to reduce testing logic to the minimum.
Tests were previously doing the same initialization. It was verbose. It has been moved in the setup of a test suite.