feat(e2e): simplify the relation between manifest and testnet by cason · Pull Request #3964 · cometbft/cometbft

cason · 2024-09-02T09:40:52Z

Refactors the e2e testbed to remove some unnecessary duplication between Manifest and Testnet, and ManifestNode and Node. Fields that are identical in the two types are removed from the latter, which includes (extends) the former. Fields that had the same name but different types or semantics in the two versions, were renamed in the manifest.

With this change, to add a new field to the configuration of a testnet or a node, it is enough to update the Manifest or ManifestNode. The new field must also be added to Testnet or Node if it needs to be somehow parsed (see the examples of the Seeds and PersistentPeers fields).

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

melekes · 2024-09-03T04:38:38Z

test/e2e/pkg/manifest.go

 	// Defaults to "validator". Full nodes do not get a signing key (a dummy key
 	// is generated), and seed nodes run in seed mode with the PEX reactor enabled.
-	Mode string `toml:"mode"`
+	ModeStr string `toml:"mode"`


What is the benefit of putting type into a variable name?

We have Testnet.Mode and had Manifest.Node. They are from different types.

Ok, it is true that type Mode = String but they are different variables.

I renamed all variables in Manifest that collide with names in Testnet.

I'm also trying to understand the motivation for these renames here, struct fields are scoped to the struct, so you can't experience 'collisions' in a sense that you can't access fields of one struct while referencing another, even if fields have the same names.

Could you please point in the code where this collision could happen ? If it's just a matter of providing context via the field name, I'd think we should just apply KISS principle and leave the way it is. But I might have missed the point with the rename.

test/e2e/pkg/testnet.go

andynog

I've reviewed and made some comments and had some questions.

andynog · 2024-09-03T13:59:16Z

test/e2e/pkg/testnet.go

+		ValidatorUpdates: map[int64]map[*Node]int64{},
+		Nodes:            []*Node{},
+	}
+	if testnet.InitialHeight == 0 {


Do we need this if in the genesis validation if it's height = 0 it changes it to height = 1 ?

cometbft/types/genesis.go

Line 80 in 3323096

if genDoc.InitialHeight == 0 {

andynog · 2024-09-03T14:14:12Z

test/e2e/pkg/manifest.go

 	// Defaults to "validator". Full nodes do not get a signing key (a dummy key
 	// is generated), and seed nodes run in seed mode with the PEX reactor enabled.
-	Mode string `toml:"mode"`
+	ModeStr string `toml:"mode"`


I'm also trying to understand the motivation for these renames here, struct fields are scoped to the struct, so you can't experience 'collisions' in a sense that you can't access fields of one struct while referencing another, even if fields have the same names.

Could you please point in the code where this collision could happen ? If it's just a matter of providing context via the field name, I'd think we should just apply KISS principle and leave the way it is. But I might have missed the point with the rename.

andynog

lgtm, overall it improves the code

Addresses #3832. Introduces a flag `config` to the manifest. It is a list of strings that should have the format `key = value`. Keys are any of the configuration parameters included in a CometBFT configuration file. The flag can be global, meaning that the configurations will be applied for every node in the network, or specific for a given node, when inserted in the section referring to that node. Local/specific configurations are applied after the global ones, therefore overriding them. Example: ``` config = [ "p2p.send_rate = 51200", "filter_peers = true", ] ``` The configurations are applying using `viper`, as I could not find another way to load configuration parameters. They are always loaded as a string, and this appears to work correctly. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

Closes #3961. Similarly to #3967, introduces a new `genesis` field for manifest files. The format is `key = value` and multiple entries can be provided. For example, ``` genesis = [ "consensus_params.evidence.max_bytes = 1000", "chain_id = pr3969" ] ``` After the genesis document is produced by the `setup` implementation of the runner, the provided genesis customized configurations are applied to the generated genesis file. The solution also uses `viper`, as it allows setting only some specific keys, leaving the pre-produced content untouched. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

As per title. It was like this before #3964, so restoring original name. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Refactors the `e2e` testbed to remove some unnecessary duplication between `Manifest` and `Testnet`, and `ManifestNode` and `Node`. Fields that are identical in the two types are removed from the latter, which includes (extends) the former. Fields that had the same name but different types or semantics in the two versions, were renamed in the manifest. With this change, to add a new field to the configuration of a testnet or a node, it is enough to update the `Manifest` or `ManifestNode`. The new field must also be added to `Testnet` or `Node` if it needs to be somehow parsed (see the examples of the `Seeds` and `PersistentPeers` fields). --- - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

Refactors the `e2e` testbed to remove some unnecessary duplication between `Manifest` and `Testnet`, and `ManifestNode` and `Node`. Fields that are identical in the two types are removed from the latter, which includes (extends) the former. Fields that had the same name but different types or semantics in the two versions, were renamed in the manifest. With this change, to add a new field to the configuration of a testnet or a node, it is enough to update the `Manifest` or `ManifestNode`. The new field must also be added to `Testnet` or `Node` if it needs to be somehow parsed (see the examples of the `Seeds` and `PersistentPeers` fields). --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev> (cherry picked from commit 40c4b40) # Conflicts: # test/e2e/generator/generate.go # test/e2e/pkg/testnet.go

Addresses #3832. Introduces a flag `config` to the manifest. It is a list of strings that should have the format `key = value`. Keys are any of the configuration parameters included in a CometBFT configuration file. The flag can be global, meaning that the configurations will be applied for every node in the network, or specific for a given node, when inserted in the section referring to that node. Local/specific configurations are applied after the global ones, therefore overriding them. Example: ``` config = [ "p2p.send_rate = 51200", "filter_peers = true", ] ``` The configurations are applying using `viper`, as I could not find another way to load configuration parameters. They are always loaded as a string, and this appears to work correctly. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev> (cherry picked from commit cea82b1)

Closes #3961. Similarly to #3967, introduces a new `genesis` field for manifest files. The format is `key = value` and multiple entries can be provided. For example, ``` genesis = [ "consensus_params.evidence.max_bytes = 1000", "chain_id = pr3969" ] ``` After the genesis document is produced by the `setup` implementation of the runner, the provided genesis customized configurations are applied to the generated genesis file. The solution also uses `viper`, as it allows setting only some specific keys, leaving the pre-produced content untouched. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev> (cherry picked from commit 6c5cd32) # Conflicts: # test/e2e/pkg/manifest.go # test/e2e/pkg/testnet.go # test/e2e/runner/setup.go

#3964) (#4308) Refactors the `e2e` testbed to remove some unnecessary duplication between `Manifest` and `Testnet`, and `ManifestNode` and `Node`. Fields that are identical in the two types are removed from the latter, which includes (extends) the former. Fields that had the same name but different types or semantics in the two versions, were renamed in the manifest. With this change, to add a new field to the configuration of a testnet or a node, it is enough to update the `Manifest` or `ManifestNode`. The new field must also be added to `Testnet` or `Node` if it needs to be somehow parsed (see the examples of the `Seeds` and `PersistentPeers` fields). --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments <hr>This is an automatic backport of pull request #3964 done by [Mergify](https://mergify.com). --------- Co-authored-by: Daniel <daniel.cason@informal.systems>

Closes #3961. Similarly to #3967, introduces a new `genesis` field for manifest files. The format is `key = value` and multiple entries can be provided. For example, ``` genesis = [ "consensus_params.evidence.max_bytes = 1000", "chain_id = pr3969" ] ``` After the genesis document is produced by the `setup` implementation of the runner, the provided genesis customized configurations are applied to the generated genesis file. The solution also uses `viper`, as it allows setting only some specific keys, leaving the pre-produced content untouched. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

…ort #3967) (#4309) Addresses #3832. Introduces a flag `config` to the manifest. It is a list of strings that should have the format `key = value`. Keys are any of the configuration parameters included in a CometBFT configuration file. The flag can be global, meaning that the configurations will be applied for every node in the network, or specific for a given node, when inserted in the section referring to that node. Local/specific configurations are applied after the global ones, therefore overriding them. Example: ``` config = [ "p2p.send_rate = 51200", "filter_peers = true", ] ``` The configurations are applying using `viper`, as I could not find another way to load configuration parameters. They are always loaded as a string, and this appears to work correctly. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments <hr>This is an automatic backport of pull request #3967 done by [Mergify](https://mergify.com). --------- Co-authored-by: Daniel <daniel.cason@informal.systems>

Closes #3961. Similarly to #3967, introduces a new `genesis` field for manifest files. The format is `key = value` and multiple entries can be provided. For example, ``` genesis = [ "consensus_params.evidence.max_bytes = 1000", "chain_id = pr3969" ] ``` After the genesis document is produced by the `setup` implementation of the runner, the provided genesis customized configurations are applied to the generated genesis file. The solution also uses `viper`, as it allows setting only some specific keys, leaving the pre-produced content untouched. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

…port #3969) (#4312) Closes #3961. Similarly to #3967, introduces a new `genesis` field for manifest files. The format is `key = value` and multiple entries can be provided. For example, ``` genesis = [ "consensus_params.evidence.max_bytes = 1000", "chain_id = pr3969" ] ``` After the genesis document is produced by the `setup` implementation of the runner, the provided genesis customized configurations are applied to the generated genesis file. The solution also uses `viper`, as it allows setting only some specific keys, leaving the pre-produced content untouched. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- This is an manual backport of pull request #3969. #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments

e2e: rename Manifest fields colliding with Testnet

4991c80

cason force-pushed the cason/e2e-refactor branch from 4bbb231 to 4991c80 Compare September 2, 2024 11:17

Daniel Cason added 3 commits September 2, 2024 13:18

e2e: Tesnet includes and extends Manifest

92f9ec1

e2e: Node includes and extends ManifestNode

0a596f2

e2e: fix Database Node setting

778fbc6

cason changed the title ~~feat(e2e): some refactor [DRAFT]~~ feat(e2e): simplify the relation between manifest and testnet Sep 2, 2024

cason marked this pull request as ready for review September 2, 2024 12:19

cason requested a review from a team as a code owner September 2, 2024 12:19

cason requested a review from a team September 2, 2024 12:19

cason mentioned this pull request Sep 2, 2024

feat(e2e): config flag for arbitrary CometBFT configurations #3967

Merged

3 tasks

cason added the e2e Related to our end-to-end tests label Sep 2, 2024

Merge branch 'main' into cason/e2e-refactor

2072bad

cason mentioned this pull request Sep 2, 2024

feat(e2e): allow customizing genesis parameters on the manifest #3969

Merged

3 tasks

melekes reviewed Sep 3, 2024

View reviewed changes

mergify bot added 3 commits September 3, 2024 08:47

Merge branch 'main' into cason/e2e-refactor

6e8de84

Merge branch 'main' into cason/e2e-refactor

71b18ae

Merge branch 'main' into cason/e2e-refactor

a476a03

andynog reviewed Sep 3, 2024

View reviewed changes

andynog and others added 3 commits September 3, 2024 15:29

Merge branch 'main' into cason/e2e-refactor

c0da912

Merge branch 'main' into cason/e2e-refactor

eb40ed4

Merge branch 'main' into cason/e2e-refactor

49dbc0d

melekes approved these changes Sep 4, 2024

View reviewed changes

andynog approved these changes Sep 4, 2024

View reviewed changes

cason added this pull request to the merge queue Sep 4, 2024

Merged via the queue into main with commit 40c4b40 Sep 4, 2024

cason deleted the cason/e2e-refactor branch September 4, 2024 13:17

cason mentioned this pull request Sep 23, 2024

refactor(e2e): change *Node to string in keys of some maps #4141

Merged

1 task

cason mentioned this pull request Sep 24, 2024

feat(e2e): Manifest, renamed ValidatorsMap -> Validators #4166

Merged

3 tasks

cason added the backport-to-v1.x label Oct 20, 2024

mergify bot mentioned this pull request Oct 20, 2024

feat(e2e): simplify the relation between manifest and testnet (backport #3964) #4308

Merged

3 tasks

mergify bot mentioned this pull request Oct 20, 2024

feat(e2e): config flag for arbitrary CometBFT configurations (backport #3967) #4309

Merged

3 tasks

mergify bot mentioned this pull request Oct 20, 2024

feat(e2e): allow customizing genesis parameters on the manifest (backport #3969) #4310

Closed

3 tasks

cason mentioned this pull request Oct 20, 2024

feat(e2e): allow customizing genesis parameters on the manifest (backport #3969) #4312

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(e2e): simplify the relation between manifest and testnet#3964

feat(e2e): simplify the relation between manifest and testnet#3964
cason merged 11 commits intomainfrom
cason/e2e-refactor

cason commented Sep 2, 2024 •

edited

Loading

Uh oh!

melekes Sep 3, 2024

Uh oh!

cason Sep 3, 2024

Uh oh!

andynog Sep 3, 2024

Uh oh!

andynog Sep 4, 2024

Uh oh!

Uh oh!

andynog left a comment

Uh oh!

andynog Sep 3, 2024

Uh oh!

andynog Sep 3, 2024

Uh oh!

andynog left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cason commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

melekes Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

cason Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

andynog Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

andynog Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andynog left a comment

Choose a reason for hiding this comment

Uh oh!

andynog Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

andynog Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

andynog left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cason commented Sep 2, 2024 •

edited

Loading