Fix flaky bootstrapping test #3955
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request re-writes the flaky bootstrapping test to verify the originally expected behavior as described in issue #3951. Key changes include:
- Updating comments to clearly describe the bootstrapping process.
- Adjusting the handling of time, message channels, and validator removal block creation.
- Replacing dynamic configurations with fixed values to ensure test consistency.
Comments suppressed due to low confidence (3)
vms/platformvm/vm_test.go:1171
- Verify that transitioning from GetConfigWithUpgradeTime to GetConfig(upgradetest.Latest) aligns with the intended upgrade behavior for this test scenario.
UpgradeConfig: upgradetest.GetConfig(upgradetest.Latest),
vms/platformvm/vm_test.go:1180
- [nitpick] Consider renaming the channel 'toEngine' to a more descriptive name that clarifies its role in passing messages to the engine.
toEngine := make(chan common.Message, 1)
vms/platformvm/vm_test.go:1295
- [nitpick] It may be helpful to include an inline comment explaining why SampleK is set to 1 in this test, providing clarity for future maintainers.
SampleK: 1,
StephenButtolph
requested review from
a team and
DracoLi
and removed request for
a team
| MinStakeDuration: defaultMinStakingDuration, | ||
| MaxStakeDuration: defaultMaxStakingDuration, | ||
| RewardConfig: defaultRewardConfig, | ||
| UpgradeConfig: upgradetest.GetConfigWithUpgradeTime(upgradetest.Durango, latestForkTime), |
There was a problem hiding this comment.
Q: Is there a reason we kept this as Durango and not latest before? I'm wondering if we should update the other tests in the file to also be the latest fork, or fine to keep them as is.
There was a problem hiding this comment.
Is there a reason we kept this as Durango and not latest before
I don't think so. Looking around in the git blame it seems like we just hadn't updated this test (and we previously didn't have the notion of the "Latest" which automatically pushes tests to the most recent upgrade).
I'm wondering if we should update the other tests in the file to also be the latest fork
I think we do probably want most of the tests to be for the latest fork. We do still want testing coverage for state transitions that we need to perform during bootstrapping... But this test specifically should always been hitting the latest ruleset (because we will have advanced to the latest ruleset by the time bootstrapping finishes)
Why this should be merged
This bootstrapping test has deviated pretty far from the original purpose of the test. This returns the test to verify the originally expected behavior, and resolves #3951.
Specifically, this test was originally added to verify that the correct preference is set if the oracle block was accepted during bootstrapping.
During the removal of the AdvanceTimestampTx, it seems that this test was modified and the oracle blocks were removed entirely (completely missing the whole point of the test).
In #3943 the test context was modified to generate the nodeID in the test context, which broke this test.
How this works
Re-writes the flaky test.
How this was tested
CI
Need to be documented in RELEASES.md?
No