Skip to content

e2e: add contexts and e2e generator tests#9426

Merged
cmwaters merged 4 commits intofeature/abci++veffrom
cal/port-e2e
Nov 10, 2022
Merged

e2e: add contexts and e2e generator tests#9426
cmwaters merged 4 commits intofeature/abci++veffrom
cal/port-e2e

Conversation

@cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Sep 13, 2022

This PR tackles some of the preliminary work in regards to the e2e outlined in #9396

@cmwaters cmwaters requested a review from ebuchman as a code owner September 13, 2022 12:53
@cmwaters cmwaters requested a review from a team September 13, 2022 12:53
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale for use by stalebot label Sep 24, 2022
@cmwaters cmwaters removed the stale for use by stalebot label Sep 26, 2022
func Benchmark(testnet *e2e.Testnet, benchmarkLength int64) error {
block, _, err := waitForHeight(testnet, 0)
func Benchmark(ctx context.Context, testnet *e2e.Testnet, benchmarkLength int64) error {
block, _, err := waitForHeight(ctx, testnet, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Small point here. This is not counted towards the test duration time. If I understand correctly, this will wait for all nodes to report they are at height 0 akka they have started etc.) Does this include other operations that are relevant to measure - creating connections between nodes for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this just simply waits until all nodes have started before beginning the actual benchmark

Copy link
Contributor

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

There are some involved chunks I didn't review (yet).
Before reviewing (potentially) already reviewed code, is this PR a result of a purely backport? Or are some things reworked?
If a backport, what PRs were used as source? (or what "git diff" command?)
I'm asking because that's the most efficient way to review a backport.


## Conceptual Overview

End-to-end testnets are used to test Tendermint functionality as a user would use it, by spinning up a set of nodes with various configurations and making sure the nodes and network behave correctly. The background for the E2E test suite is outlined in [RFC-001](https://github.com/tendermint/tendermint/blob/master/docs/architecture/adr-066-e2e-testing.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

RFC001 seems to refer to storage, not e2e. Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I guess this was missed in the earlier PR

@cmwaters
Copy link
Contributor Author

This PR isn't based on cherry-picked commits. There were probably a handful of PRs. I've just directly looked at the diff between v0.36 and main and copied the relevant items across. This seems to be adding contexts to e2e methods, a test for the generator, more documentation and addition of extra data in the benchmark command

@sergio-mena
Copy link
Contributor

This PR isn't based on cherry-picked commits. There were probably a handful of PRs. I've just directly looked at the diff between v0.36 and main and copied the relevant items across. This seems to be adding contexts to e2e methods, a test for the generator, more documentation and addition of extra data in the benchmark command

What was the exact "git diff" you ran? (IOW, which two hashes were you comparing?)

@cmwaters
Copy link
Contributor Author

cmwaters commented Oct 6, 2022

What was the exact "git diff" you ran? (IOW, which two hashes were you comparing?)

main and v0.36.x. (do you want the exact hashes?)

@sergio-mena
Copy link
Contributor

do you want the exact hashes?

A priori, not necessary, I'll "git diff" using the tip of both branches. If I run into ambiguities, I'll let you know.
Hopefully, I'll be able to tackle this next week.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale for use by stalebot label Oct 17, 2022
@thanethomson thanethomson removed the stale for use by stalebot label Oct 21, 2022
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

It seems like all this change does is plumb contexts through the E2E tests?

If so, the issue title and description don't really match up with what's being done here. Are there other changes from v0.36 that will eventually be ported across?

@sergio-mena
Copy link
Contributor

sergio-mena commented Oct 21, 2022

Sorry for taking some to time come back to this.
I compared the diff between v0.36.x and main, with this PR. There seem to be large parts that are not present in this PR. I can understand that the vote extension related ones don't make sense (yet), but what about the others?

@cmwaters
Copy link
Contributor Author

It seems like all this change does is plumb contexts through the E2E tests?

That's the crux of it. There were some more documenting changes; a test was added to ensure that all generated testnets are valid; modifications to how we wait for nodes to reach a certain height was adjusted (a lot of small stuff)

If so, the issue title and description don't really match up with what's being done here. Are there other changes from v0.36 that will eventually be ported across?

Yeah, there are still a chunk of changes that can only happen once finalize block and vote extensions have actually been implemented. This PR is supposed to address the "preliminary work" within e2e as outlined by Sergio. There will definitely need to be a second PR later on. I'm fine with renaming it to something more scoped.

@cmwaters cmwaters changed the title e2e: port across changes from v0.36 e2e: add contexts and e2e generator tests Oct 26, 2022
PrivValServer string `toml:"privval_server"`
PrivValKey string `toml:"privval_key"`
PrivValState string `toml:"privval_state"`
Misbehaviors map[string]string `toml:"misbehaviors"`
Copy link
Contributor

Choose a reason for hiding this comment

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

So this doesn't currently support misbehavior? (Doesn't look like this was used anywhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think this was vestige from v0.34 back when the maverick node was around. It's unused.

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale for use by stalebot label Nov 8, 2022
@sergio-mena sergio-mena removed the stale for use by stalebot label Nov 9, 2022
@cmwaters cmwaters merged commit f8e453a into feature/abci++vef Nov 10, 2022
@cmwaters cmwaters deleted the cal/port-e2e branch November 10, 2022 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done/Merged

Development

Successfully merging this pull request may close these issues.

4 participants