Remove duplicate genesis creations in P-chain unit tests #3318
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dhrubabasu
approved these changes
Aug 21, 2024
StephenButtolph
commented
Aug 22, 2024
Comment on lines
-518
to
-524
| // Staker2's end time matches staker5's start time, so typically | ||
| // the block builder would produce a ProposalBlock to remove | ||
| // staker2 when advancing the time. However, this test injects | ||
| // staker0 into the staker set artificially to advance the time. | ||
| // This means that staker2 is not removed by the ProposalBlock | ||
| // when advancing the time. |
There was a problem hiding this comment.
This comment is describing that the behavior of this test makes assumptions on the hash ordering of txID for staker0 and staker2. This is not useful to test as it is currently written imo, so decided to remove it.
StephenButtolph
commented
Aug 22, 2024
| @@ -137,7 +138,7 @@ func TestRewardValidatorTxExecuteOnCommit(t *testing.T) { | |||
|
|
|||
| onCommitBalance, err := avax.GetBalance(env.state, stakeOwners) | |||
| require.NoError(err) | |||
| require.Equal(oldBalance+stakerToRemove.Weight+27697, onCommitBalance) | |||
| require.Equal(oldBalance+stakerToRemove.Weight+38944, onCommitBalance) | |||
There was a problem hiding this comment.
This is a terrible test. But out-of-scope for this PR.
There was a problem hiding this comment.
(No action required) What does this magic number signify?
There was a problem hiding this comment.
This magic number is signifying the staking rewards that the genesis validator will receive. The reason this number changes is because the genesis staking duration was increased from 20 days to 28 days (because 28 days was used in most other places)
StephenButtolph
commented
Aug 22, 2024
| @@ -754,71 +750,3 @@ func buildVM(t *testing.T) (*VM, ids.ID, error) { | |||
|
|
|||
| return vm, testSubnet1.ID(), nil | |||
| } | |||
|
|
|||
| func buildCustomGenesis(avaxAssetID ids.ID) ([]byte, error) { | |||
| // Returns: | ||
| // 1) The genesis state | ||
| // 2) The byte representation of the default genesis for tests | ||
| func defaultGenesis(t *testing.T, avaxAssetID ids.ID) (*api.BuildGenesisArgs, []byte) { |
| @@ -342,60 +320,3 @@ func defaultFx(t *testing.T, clk *mockable.Clock, log logging.Logger, isBootstra | |||
| } | |||
| return res | |||
| } | |||
|
|
|||
| func buildGenesisTest(t *testing.T, ctx *snow.Context) []byte { | |||
| @@ -372,69 +347,6 @@ func defaultFx(clk *mockable.Clock, log logging.Logger, isBootstrapped bool) fx. | |||
| return res | |||
| } | |||
|
|
|||
| func buildGenesisTest(ctx *snow.Context) []byte { | |||
| @@ -322,66 +302,3 @@ func defaultFx(clk *mockable.Clock, log logging.Logger, isBootstrapped bool) fx. | |||
| } | |||
| return res | |||
| } | |||
|
|
|||
| func buildGenesisTest(ctx *snow.Context) []byte { | |||
|
|
||
| buildGenesisResponse := api.BuildGenesisReply{} | ||
| platformvmSS := api.StaticService{} | ||
| require.NoError(platformvmSS.BuildGenesis(nil, &buildGenesisArgs, &buildGenesisResponse)) |
StephenButtolph
changed the title
marun
approved these changes
Aug 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this should be merged
There are currently 6 instances of the same (slightly modified) piece of code. This unifies all of them with the
genesistest.Newfunction.How this works
How this was tested