Skip to content
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

e2e: add abci delays #9254

Merged
merged 3 commits into from
Aug 19, 2022
Merged

e2e: add abci delays #9254

merged 3 commits into from
Aug 19, 2022

Conversation

cmwaters
Copy link
Contributor

This ports the changes made in: #8638

This does not completely port over the changes as there are also delays to vote extensions and finalize block which haven't been captured. I will open an issue to track those

@cmwaters cmwaters requested a review from a team August 15, 2022 12:07
@cmwaters cmwaters changed the base branch from main to feature/abci++ppp August 15, 2022 12:08
Comment on lines 63 to 65
PrepareProposalDelayMS int
ProcessProposalDelayMS int
CheckTxDelayMS int
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use durations directly for the type?
Additionally, consider using a struct to contain these.

type ABCIDelays struct {
PrepareProposal
...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does TOML support durations or are you suggesting I just convert it after parsing the toml file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our toml parser supports parsing duration strings into go durations.

see

timeout_propose_delta = "500ms"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me change it up then

@@ -5,6 +5,7 @@ import (
"math/rand"
"sort"
"strings"
"time"

e2e "github.com/tendermint/tendermint/test/e2e/pkg"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testnetCombinations defined in line 15 was adding the possibility to test "builtin" and "out of process".
This also modified nodeABCIProtocols in line 29 (BTW, looks like we are not testing GRPC as it is now?).
Do we want to backport this change in structure of the variables controlling the test combinations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gRPC seems to have been purposefully disabled due to #5439. It's hard to judge whether this is still an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can look to port more of the e2e functionality later. I kind of want to pick off important chunks first

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Anyway remember that this effort is not in the critical path (i.e., it's not blocking the creation of branch v0.37.x). IOW, it can proceed along with the release QA process

@cmwaters cmwaters merged commit 0ca3a89 into feature/abci++ppp Aug 19, 2022
@cmwaters cmwaters deleted the cal/e2e-delay branch August 19, 2022 10:00
@sergio-mena
Copy link
Contributor

Contributes to #9053

@sergio-mena sergio-mena mentioned this pull request Aug 19, 2022
35 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants