-
Notifications
You must be signed in to change notification settings - Fork 662
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
Write a unit test for the VM Initializer #3180
base: master
Are you sure you want to change the base?
Conversation
initialized: make(chan struct{}), | ||
} | ||
} | ||
|
||
func (i *initializer) Initialize(_ context.Context, protocolVersion uint, vmAddr string) error { | ||
i.once.Do(func() { | ||
if version.RPCChainVMProtocol != protocolVersion { | ||
i.err = fmt.Errorf("%w. AvalancheGo version %s implements RPCChainVM protocol version %d. The VM implements RPCChainVM protocol version %d. Please make sure that there is an exact match of the protocol versions. This can be achieved by updating your VM or running an older/newer version of AvalancheGo. Please be advised that some virtual machines may not yet support the latest RPCChainVM protocol version", |
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.
(No action required) Maybe limit this change to the inclusion of the VM path? Using a custom error type and verifying that it matches in unit testing might make sense if we're expecting variation, but for a single call site we can verify by inspection pretty easily that we're doing what we intend while minimizing the code required.
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 do agree with your point that single-instance error shouldn't become a custom error object. The one exception that I can see here is that this error object carries multiple data elements.
Keeping these data elements decoupled from the string would allow us to log and index these ( which, in turn, would enable us to better our data-driven error mitigation )
Signed-off-by: Tsachi Herman <24438559+tsachiherman@users.noreply.github.com>
} | ||
|
||
// TestInitializerFail tests that initialization fails for unexpected versions. | ||
func TestInitializerFail(t *testing.T) { |
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.
(No action required) Is this still necessary given that the second case of TestInitializerVersions
is already testing for a bad version? Given that there is only a single version supported at a time, I'm not sure why we need to validate a bunch of bad versions.
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.
My goal with this test was to explicitly test wide range of "other" versions ( i.e. fuzzing ).
However, given that there is only a single "positive" path, I felt that it wouldn't be a sufficiently useful to just pick a version number at random. hence, I attempted to cover the range of [0..10 * expected version ].
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.
Given that the version only ever increments, any value less than the current value is going to work.
Why this should be merged
This PR adds a unit test to the VM initializer
How this works
Trivial
How this was tested
Manually.
See recent vm initializer changes here : #3178