Generalized Multinode unit tests BCI-2283 #11066

dhaidashenko · 2023-10-24T17:19:56Z

This PR is continuation of the efforts to generalize Multi-Node Failover logic:

Migrated/Reimplemented core/chains/evm/client tests to common/client
Introduced minor changes to common/client implementation to simplify testing
Moved some of the utils from to core/internal/testutils/testutils.go to the new package core/testutils to be able to use them in common/client. core/internal/testutils API stays the same

github-actions · 2023-10-24T17:20:17Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

github-actions · 2023-10-24T17:20:18Z

I see that you haven't updated any README files. Would it make sense to do so?

prashantkumar1982

Looks good to me. I can approve once changes are ready

dimriou · 2023-11-01T18:16:16Z

IMO mocks should be moved to a separate folder because right now the client seems a bit too cluttered with all the mock files.

dhaidashenko · 2023-11-01T18:27:46Z

IMO mocks should be moved to a separate folder because right now the client seems a bit cluttered with all the mock files.

Agree, but we'll have to keep SendOnlyNode, Node, NodeClient in the same package to prevent import cycle

dhaidashenko · 2023-11-02T11:52:28Z

IMO mocks should be moved to a separate folder because right now the client seems a bit cluttered with all the mock files.

Agree, but we'll have to keep SendOnlyNode, Node, NodeClient in the same package to prevent import cycle

Tried to implement it and not happy with the results. 5/8 mocks have to stay in the same package. It's still cluttered but now also more confusing, so I'd rather leave it as is.

There is an open issue in mockery to generate all mocks into single file

core/testutils/sync.go

common/client/node.go

common/client/utils_test.go

jmank88 · 2023-11-02T12:39:25Z

There is an open issue in mockery to generate all mocks into single file

v3 looks like a major improvment https://github.com/vektra/mockery#v3

dimriou · 2023-11-02T13:11:37Z

IMO mocks should be moved to a separate folder because right now the client seems a bit cluttered with all the mock files.

Agree, but we'll have to keep SendOnlyNode, Node, NodeClient in the same package to prevent import cycle

Tried to implement it and not happy with the results. 5/8 mocks have to stay in the same package. It's still cluttered but now also more confusing, so I'd rather leave it as is.

There is an open issue in mockery to generate all mocks into single file

Would moving some interfaces into different packages (perhaps the type.go file) solve the issue? If not, can we at least drop the _test suffix on the mock files? I get confused with the actual test files. I think node, mock_node, and node_test would be easier to read.

dimriou · 2023-11-02T14:32:42Z

common/client/multi_node.go

@@ -114,6 +95,7 @@ type multiNode[
 	leaseDuration       time.Duration
 	leaseTicker         *time.Ticker
 	chainFamily         string
+	reportInterval      time.Duration


I don't see any benefits in storing the const variable here. I slightly prefer the old way. Also, we can avoid having to set it on the multi_node_test.go (https://github.com/smartcontractkit/chainlink/pull/11066/files#diff-a9fee197ea880bb302ca4c0f830d292355586dca02d46c826becda641404cf30R232), since it's not used anywhere.

Yes, const version is cleaner.
I've changed it to be able to override it in tests as original value is 6.5 seconds. Two tests has to wait for ticker to fire several times increasing total duration of tests from 4 seconds to ~13 seconds. This is not a lot for general run, but it makes flake tests hunting slightly more difficult as go test -n 100 ... takes much more time to complete.

common/client/node_lifecycle.go

common/types/test_utils.go

dimriou · 2023-11-02T15:25:20Z

core/testutils/sync.go

Not sure why only some of them were moved here. If there aren't any blockers I would keep them under the same file or use the chainlink-relay pkg.

@dimriou do you mean merging logs.go and sync.go files or moving rest of the core/internal/testutils/testutils.go to the core/testutils

Three of these things exist in chailnink-relay/pkg/utils/tests:
https://github.com/smartcontractkit/chainlink-relay/blob/02b76df27fb4369ee017ce5512df1f8041f1afce/pkg/utils/tests/tests.go
Edit: Oh this is the one I already commented on. I think @dimriou and I are saying the same thing. Priority is to not duplicate the ones you can just import. Separately we could consider moving some to chainlink-relay, but it's not necessary so nbd 🤷

Ah this package is still in a weird place though. We can't tangle the imports up like this. We need to maintain the ability to move the common subtree out of this repo easily (and soon!). For that reason, it would be better to move these helpers to chainlink-relay.

Moved missing parts to chainlink-relay and switched new tests to use it. @jmank88 Should I also deprecate testutils functions?

Might want to wait since our linter is strict about using things marked deprecated.

dimriou · 2023-11-02T15:28:12Z

core/internal/testutils/testutils.go

I've also made a comment below. This doesn't seem right. We just created multiple pass-through methods.

The idea behind this function is to protect users from future changes/extensions to ID interface

common/client/mock_hashable_test.go

dhaidashenko · 2023-11-03T12:07:40Z

IMO mocks should be moved to a separate folder because right now the client seems a bit cluttered with all the mock files.

Agree, but we'll have to keep SendOnlyNode, Node, NodeClient in the same package to prevent import cycle

Tried to implement it and not happy with the results. 5/8 mocks have to stay in the same package. It's still cluttered but now also more confusing, so I'd rather leave it as is.
There is an open issue in mockery to generate all mocks into single file

Would moving some interfaces into different packages (perhaps the type.go file) solve the issue? If not, can we at least drop the _test suffix on the mock files? I get confused with the actual test files. I think node, mock_node, and node_test would be easier to read.

Yes, we'll be able to generate mocks in separate package if we move Head, NodeClient, sendOnlyClient, connection. But it seems like reducing number of mock files in the package is not important enough to change the API.

We can build some simple tool that would merge generated mock files into single file, but it's out of scope for this PR.

We have to use _test suffix to ensure that go coverage is not including these files during coverage calculations.

common/client/multi_node_test.go

dimriou · 2023-11-07T11:53:43Z

common/client/multi_node_test.go

+		assert.EqualError(t, err, expectedError.Error())
+	})
+
+	t.Run("Closes started nodes on failure", func(t *testing.T) {


I think this name is a bit misleading. During Dial if a node tries to get started and it fails for any reason besides a wrong chainID, it will simply return an error. The node was never properly started to be closed. Perhaps something like this would be more accurate:

Suggested change

t.Run("Closes started nodes on failure", func(t *testing.T) {

t.Run("Fails if at least one node fails", func(t *testing.T) {

We test multinode failure on node failure above. Here the idea was to ensure that we properly release resources of node1, that was started if node2 fails. newHealthyNode adds expectation of Close call

I see. newHealthyNode seemed like a method that creates a mock node with the right configs. The expectation of Start and Close was a bit more difficult to figure out.

common/client/multi_node_test.go

Co-authored-by: Dimitris Grigoriou <[email protected]>

# Conflicts: # core/scripts/go.mod # core/scripts/go.sum # go.mod # go.sum # integration-tests/go.mod # integration-tests/go.sum

dimriou

LGTM

cl-sonarqube-production · 2023-11-09T18:23:15Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

prashantkumar1982 · 2023-11-09T04:03:55Z

common/client/send_only_node.go

@@ -143,6 +146,7 @@ func (s *sendOnlyNode[CHAIN_ID, RPC]) start(startCtx context.Context) {
 func (s *sendOnlyNode[CHAIN_ID, RPC]) Close() error {
 	return s.StopOnce(s.name, func() error {
 		s.rpc.Close()
+		close(s.chStop)


nit: was chStop being used before? Seems unused to me.

dhaidashenko changed the base branch from develop to generalize_multinode October 24, 2023 17:20

prashantkumar1982 reviewed Oct 25, 2023

View reviewed changes

dhaidashenko force-pushed the feature/BCI-2283-generalized-multinode-tests branch from 13b8663 to 98cea17 Compare November 1, 2023 16:58

Base automatically changed from generalize_multinode to develop November 1, 2023 20:56

dhaidashenko force-pushed the feature/BCI-2283-generalized-multinode-tests branch from 9452e36 to 71cec47 Compare November 2, 2023 11:54

dhaidashenko changed the title ~~Feature/bci 2283 generalized multinode tests~~ Generalized Multinode unit tests BCI-2283 Nov 2, 2023

dhaidashenko marked this pull request as ready for review November 2, 2023 12:22

dhaidashenko requested review from a team, samsondav and jmank88 as code owners November 2, 2023 12:22

dhaidashenko requested a review from prashantkumar1982 November 2, 2023 12:23

jmank88 reviewed Nov 2, 2023

View reviewed changes

core/testutils/sync.go Outdated Show resolved Hide resolved

jmank88 reviewed Nov 2, 2023

View reviewed changes

common/client/node.go Outdated Show resolved Hide resolved

dimriou reviewed Nov 2, 2023

View reviewed changes

common/client/utils_test.go Outdated Show resolved Hide resolved

dimriou reviewed Nov 2, 2023

View reviewed changes

common/client/node_lifecycle.go Outdated Show resolved Hide resolved

dimriou reviewed Nov 2, 2023

View reviewed changes

common/client/node_lifecycle.go Outdated Show resolved Hide resolved

dimriou reviewed Nov 2, 2023

View reviewed changes

common/types/test_utils.go Outdated Show resolved Hide resolved

dimriou reviewed Nov 2, 2023

View reviewed changes

common/types/test_utils.go Outdated Show resolved Hide resolved

dimriou reviewed Nov 2, 2023

View reviewed changes

jmank88 reviewed Nov 3, 2023

View reviewed changes

common/client/mock_hashable_test.go Outdated Show resolved Hide resolved

sendOnly & node tests fixes

9804373

dhaidashenko force-pushed the feature/BCI-2283-generalized-multinode-tests branch from 8bd5418 to 9804373 Compare November 6, 2023 21:26

dhaidashenko added 5 commits November 6, 2023 22:36

make gomodtidy

aa3848f

fix deps

b7c2317

update relay

ac225dd

fix silly cleanup issue

7889b11

Merge develop into feature/BCI-2283-generalized-multinode-tests

03a6c2d