test!: adds the ability to set or unset bbr in e2e tests #3817
Conversation
staheri14
added
bug
|
Can confirm that all e2e tests work fine. |
…sing testnet fields
|
UPDATE [This issue is resolved, please skip the comment] After extensive debugging on this PR, I identified the reason behind the failure of the Context and Reproducibility: To reproduce the issue, run the The logs of the validator indicate pod initialization failed which may not provide much insight, but using Lens revealed more details about the cause: You can also reproduce the issue by running You may change the seed in the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
test/e2e/simple.go (1)
29-31: Clarify the purpose of the boolean parameter inCreateGenesisNodes.The addition of the boolean parameter to
CreateGenesisNodesis crucial for meeting the PR's objectives. However, it would be beneficial to add a comment explaining what this parameter does, especially since it's set totrue. This will improve code readability and maintainability.The code changes are approved.
Consider adding a comment to explain the purpose of the boolean parameter:
+ // Set the last parameter to true to disable BBR for genesis nodes testNet.CreateGenesisNodes(4, latestVersion, 10000000, 0, testnet.DefaultResources, true))test/e2e/benchmark/manifest.go (1)
108-117: Approve the changes in thesummarymethod and suggest adding explanatory comments.The updated
summarymethod correctly incorporates theDisableBBRfield to adjust the output based on the BBR settings. Adding comments to explain the conditional logic would enhance code readability.The code changes are approved.
Consider adding comments to explain the conditional logic:
if m.DisableBBR { + // Disable BBR: set latency to 0 latency = 0 }test/e2e/minor_version_compatibility.go (1)
58-60: Clarify the purpose of the boolean parameter inCreateGenesisNode.The addition of the boolean parameter to
CreateGenesisNodeis crucial for meeting the PR's objectives. However, it would be beneficial to add a comment explaining what this parameter does, especially since it's set tofalse. This will improve code readability and maintainability.The code changes are approved.
Consider adding a comment to explain the purpose of the boolean parameter:
+ // Set the last parameter to false to disable BBR for genesis nodes testNet.CreateGenesisNode(v, 10000000, 0, testnet.DefaultResources, false))
|
I ran the following tests once more after merging the main branch into this PR and can confirm they all work fine:
|
Agreed! Might just be easier to simplify. |
| testnet.NoError("failed to create genesis node", testNet.CreateGenesisNode(v, 10000000, 0, testnet.DefaultResources)) | ||
| testnet.NoError("failed to create genesis node", | ||
| testNet.CreateGenesisNode(v, 10000000, 0, | ||
| testnet.DefaultResources, false)) |
There was a problem hiding this comment.
Not blocking, but I think we can simply always use the no bbr flag here
We cannot default to true i.e., disabling bbr since in the versions used in MinorVersionCompatibility, this flag is not present yet hence causing failure in the test. cc: @evan-forbes @ninabarbakadze
There was a problem hiding this comment.
To elaborate on my previous comment: my perspective is that the bbr flag must be set according to the version of the celestia-app used for the validators. Since some versions support this flag while others do not, it is being made a parameter in the CreateGenesisNode function. This allows the caller, based on the version specified (the first parameter), to determine whether to enable or disable bbr.
|
The linter complaint is not caused by this PR. |
Closes #3815 by disabling BBR in e2e tests.