-
Notifications
You must be signed in to change notification settings - Fork 278
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
test!: adds the ability to set or unset bbr in e2e tests #3817
Conversation
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 |
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.
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
CreateGenesisNodes
is 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 thesummary
method and suggest adding explanatory comments.The updated
summary
method correctly incorporates theDisableBBR
field 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
CreateGenesisNode
is 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:
|
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.
Not blocking, but I think we can simply always use the no bbr flag here
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.