-
Notifications
You must be signed in to change notification settings - Fork 841
Fully populate test context #3943
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
Conversation
| // Ensure that the blocks have at least one bit as a common prefix | ||
| for firstBlock.IDV.Bit(0) != secondBlock.IDV.Bit(0) { | ||
| secondBlock = snowmantest.BuildChild(snowmantest.Genesis) | ||
| } |
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.
This test previously assumed that this was the case. In practice it was because all of our tests are deterministic, but adding an additional call to ids.GenerateTestNodeID() changed the seed.
This should now work generally.
| BCLookup: aliaser, | ||
| Metrics: metrics.NewPrefixGatherer(), | ||
|
|
||
| WarpSigner: warp.NewSigner(secretKey, constants.UnitTestID, chainID), |
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.
Was nil previously.
| BCLookup: aliaser, | ||
| Metrics: metrics.NewPrefixGatherer(), | ||
| Log: logging.NoLog{}, | ||
| SharedMemory: sharedMemory, |
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.
Was nil previously.
| SubnetID: constants.PrimaryNetworkID, | ||
| ChainID: chainID, | ||
| NodeID: ids.EmptyNodeID, | ||
| NodeID: ids.GenerateTestNodeID(), |
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.
Was invalid previously. We will never populate the empty ID here in prod.
| CChainID: constants.PrimaryNetworkID, | ||
| }[chainID] | ||
| if !ok { | ||
| switch chainID { |
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.
Technically not required, but during my canoto work, I realized how crazy inefficient this map lookup is... It is also less code and probably more readable tbh
| ) | ||
|
|
||
| var ( | ||
| PChainID = constants.PlatformChainID |
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.
Just wanted the order to align with the rest of the file... Not really needed
Why this should be merged
Currently coreth has a ton of duplicate code around the handling of contexts. This change should help use this code there.
How this works
How this was tested
Need to be documented in RELEASES.md?