refactor(e2e): change *Node to string in keys of some maps#4141
refactor(e2e): change *Node to string in keys of some maps#4141sergio-mena merged 11 commits intomainfrom
*Node to string in keys of some maps#4141Conversation
andynog
left a comment
There was a problem hiding this comment.
lgtm, had one comment but I think it is irrelevant to the current logic so feel free to disregard.
| // It includes all fields from the associated Manifest instance. | ||
| type Testnet struct { | ||
| Manifest | ||
| *Manifest |
There was a problem hiding this comment.
Are you sure this doesn't break anything?
There was a problem hiding this comment.
It was breaking when not used as a pointer, because we would copy some fields (non-pointer fields) but not others. A total mess.
As we are now modifying the Manifest down below in this function, those modifications didn't propagate (hence I needed to turn it into a pointer)
| for valName, power := range *testnet.Manifest.ValidatorsMap { | ||
| validator := testnet.LookupNode(valName) | ||
| if validator == nil { | ||
| return types.GenesisDoc{}, fmt.Errorf("unknown validator %q for genesis doc", valName) |
| for valName, power := range validators { | ||
| validator := node.Testnet.LookupNode(valName) | ||
| if validator == nil { | ||
| return nil, fmt.Errorf("unknown validator %q for validator updates in testnet, height %d", valName, height) |
There was a problem hiding this comment.
Should never happen too, should we panic?
There was a problem hiding this comment.
I'd rather return the error and have the callsite(s) decide whether they want to panic or not
|
Consider merging #4166 into this. |
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>
Partially addresses #2458
Change the
*Nodeas keys of some maps ine2eand replace them by the node's name.Having pointers as keys of a map brings about subtleties (how comparison is done) that we don't need. Code is much clearer with node names, even if it takes a bit longer to execute some loops (we need to look up the nodes). But this is test code, so clarity and robustness comes before performance of the testing code.
I decided to tackle this refactor in an different PR, rather than piggybacking it to #2283
PR checklist
[ ] Changelog entry added in.changelog(we use unclog to manage our changelog)[ ] Updated relevant documentation (docs/orspec/) and code comments