refactor: Consolidate node-related fields into a struct #3232
Conversation
islamaliev
added
area/testing
islamaliev
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3232 +/- ##
===========================================
- Coverage 77.53% 77.51% -0.02%
===========================================
Files 382 382
Lines 35263 35263
===========================================
- Hits 27341 27334 -7
- Misses 6296 6302 +6
- Partials 1626 1627 +1
Flags with carried forward coverage won't be shown. Click here to find out more. see 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
tests/integration/state.go
Outdated
| // The node active in this test. | ||
| node *node.Node | ||
| // The node's client active in this test. | ||
| client clients.Client |
There was a problem hiding this comment.
suggestion: If nodeState embeds *node.Node calling node.client the node will be a little nicer. E.g.:
type nodeState struct {
*node.Node
...
}
suggestion: node.Node and clients.Client seem to basically be the same thing, it looks like it would be minimal hassle to get rid of clients.Client and just call nodeState.node.Peer.Connect(...).
There was a problem hiding this comment.
suggestion: node.Node and clients.Client seem to basically be the same thing, it looks like it would be minimal hassle to get rid of clients.Client and just call nodeState.node.Peer.Connect(...).
You mean to get rid of nodeState's client and instead of for example:
node.client.GetCollections(s.ctx, client.CollectionFetchOptions{})call
node.DB.GetCollections(s.ctx, client.CollectionFetchOptions{})(assuming your first suggestion is applied).
There was a problem hiding this comment.
Ah - my bad, when I made that suggestion I though node.Node implemented client.DB and you could have done node.GetCollections. Don't bother with that suggestion :)
tests/integration/utils.go
Outdated
| if len(s.nodeConfigs) == 0 { | ||
| // If there are no explicit node configuration actions the node will be | ||
| // basic (i.e. no P2P stuff) and can be yielded now. | ||
| c, err := setupClient(s, node) | ||
| require.NoError(s.t, err) | ||
| s.nodes[nodeIndex] = c | ||
|
|
||
| eventState, err := newEventState(c.Events()) | ||
| require.NoError(s.t, err) | ||
| s.nodeEvents[nodeIndex] = eventState | ||
| continue | ||
| } | ||
|
|
||
| // We need to make sure the node is configured with its old address, otherwise | ||
| // a new one may be selected and reconnection to it will fail. | ||
| var addresses []string | ||
| for _, addr := range s.nodeAddresses[nodeIndex].Addrs { | ||
| addresses = append(addresses, addr.String()) | ||
| } | ||
|
|
||
| nodeOpts := s.nodeConfigs[nodeIndex] | ||
| nodeOpts = append(nodeOpts, net.WithListenAddresses(addresses...)) | ||
|
|
||
| node.Peer, err = net.NewPeer(s.ctx, node.DB.Blockstore(), node.DB.Encstore(), node.DB.Events(), nodeOpts...) | ||
| require.NoError(s.t, err) |
There was a problem hiding this comment.
question: What happened to this logic?
There was a problem hiding this comment.
it's all inside setupNode
islamaliev
force-pushed
the
refactor/consolidate-node-fields-in-test
branch
from
e24d2df to
2a010ba
Compare
There was a problem hiding this comment.
Overall this is a nice change Islam. There are a few issues that I've spotted that caused the tests you pointed out to me to fail. You'll find todos in the relavent areas.
One thing I want to highlight is that this PR managed to make all ci test jobs use the go client regardless of the client selected by the matrix. It would be nice if we could add some kind of validation to prevent this form happening again (out of scope)
While troubleshooting, I've applied changes that fix the issues so if you want to use any or all of it feel free to do so: b3f4fbe#diff-31fde4ee4ab77f7265d4f25d4941c99e626d8b1969c9ff3667e1c6f0c7c4878f
I also noticed that even after fixing, some of the test were a little flaky due to Dial backoff. I'm not sure yet what to do about this but if it turns out to be a problem in the CI, I'll look deeper into it.
| event: eventState, | ||
| p2p: newP2PState(), | ||
| dbPath: path, | ||
| netOpts: netOpts, |
There was a problem hiding this comment.
todo: netOpts is store on the struct but never reused. These options should be passed to setupNode on restart.
islamaliev
force-pushed
the
refactor/consolidate-node-fields-in-test
branch
from
2a010ba to
8c953d4
Compare
Relevant issue(s)
Resolves #3208
Description
All node-related fields are moved into a separate
nodeStatestruct so now we don't need to maintain all slices and node indexes and it's also easier to reason about a node's state.