test: Add integration testing for P2P. #655

fredcarle · 2022-07-23T07:49:09Z

Relevant issue(s)

Resolves #564

Description

This PR adds P2P integration testing functionalities. It's limited to passive syncing of nodes on a given document. The test suite starts 2 Defra nodes, waits for them to connect and then proceeds to update the commonly shared document. We then ensure that the changes on one node are reflected on the other.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Integration tests

Specify the platform(s) on which this was tested:

MacOS

jsimnz

Looking good! 👍

Have some suggestions on the event/notification system, and the use of the GRPC stuff.

net/server.go

node/node.go

jsimnz · 2022-07-23T21:06:54Z

node/node.go

+}
+
+// SubsribeToPeerConnectionEvents subscribes the node to the event bus for a peer connection change.
+func (n *Node) SubsribeToPeerConnectionEvents() {


suggestion(blocking): I'm not sure this needs to be public. It can be set internally during Node startup.

I can be internal but that would mean that we would always be subscribing to it on startup. Is that something we want?

Thats a good point. I'm also not sure regarding the EventBus system if Emit calls will block if there is a cooresponding Subscribe channel created for the given type. Which might be problematic. Let me investigate.

After some testing, it looks like everything is working fine, even with the subscribers running without anything listening on the other side.

node/node.go

tests/integration/net/tcp_test.go

tests/integration/net/utils.go

codecov · 2022-07-25T03:13:59Z

Codecov Report

Merging #655 (624bc67) into develop (7a627d6) will increase coverage by 4.40%.
The diff coverage is 52.41%.

❗ Current head 624bc67 differs from pull request most recent head d631f26. Consider uploading reports for the commit d631f26 to get more accurate results

@@             Coverage Diff             @@
##           develop     #655      +/-   ##
===========================================
+ Coverage    57.30%   61.71%   +4.40%     
===========================================
  Files          122      123       +1     
  Lines        14783    14895     +112     
===========================================
+ Hits          8471     9192     +721     
+ Misses        5585     4847     -738     
- Partials       727      856     +129

Impacted Files	Coverage Δ
merkle/crdt/merklecrdt.go	`60.37% <0.00%> (+37.73%)`	⬆️
net/peer.go	`31.33% <33.33%> (+18.61%)`	⬆️
net/server.go	`57.47% <48.14%> (+43.79%)`	⬆️
node/node.go	`60.10% <55.29%> (-4.37%)`	⬇️
db/collection.go	`65.61% <100.00%> (+5.81%)`	⬆️
merkle/clock/heads.go	`73.45% <100.00%> (ø)`
net/utils/util.go	`20.68% <0.00%> (ø)`
config/config.go	`73.30% <0.00%> (+0.88%)`	⬆️
datastore/wrappedStore.go	`64.23% <0.00%> (+1.45%)`	⬆️
logging/logger.go	`76.35% <0.00%> (+1.51%)`	⬆️
... and 16 more

net/server.go

node/node.go

jsimnz · 2022-07-25T07:40:07Z

node/node.go

+}
+
+// SubsribeToPeerConnectionEvents subscribes the node to the event bus for a peer connection change.
+func (n *Node) SubsribeToPeerConnectionEvents() {


Thats a good point. I'm also not sure regarding the EventBus system if Emit calls will block if there is a cooresponding Subscribe channel created for the given type. Which might be problematic. Let me investigate.

jsimnz

contexts, contexts, contexts 😂

db/collection.go

node/node.go

tests/integration/net/utils.go

tests/integration/net/tcp_test.go

tests/integration/net/utils.go

merkle/crdt/merklecrdt.go

tests/integration/net/utils.go

orpheuslummis · 2022-07-25T16:41:05Z

idea: We can break the requirement on an ordering of nodes if we obtain all nodes' PeerIDs and ports beforehand. That would simplify the DX of constructing such tests. The PeerID is a deterministic construction from a node's public key. A refactor of the key system used in the node package would potentially allow us to obtain all the nodes' PeerIDs from the beginning. (an idea not for this PR, but to consider about this integration test suite generally)

orpheuslummis · 2022-07-25T16:49:04Z

idea: A potential area of extension is a syntax for defining topologies and determining behavior of its links (mock network layer).

node/node.go

tests/integration/net/utils.go

orpheuslummis · 2022-07-25T18:12:06Z

thought: if the test suite would be async, not sequential, the timeouts would gain more meaning as failure condition.

orpheuslummis

LGTM, provided the issue with usedPorts is fixed

shahzadlone

Thanks for setting up the integration testing for P2P stuff and the 4%+ bump in code coverage.

- change Seeds to SeedDocuments - Remove description and replace with comment

Relevant issue(s) Resolves sourcenetwork#564 Description This PR adds P2P integration testing functionalities. It's limited to passive syncing of nodes on a given document. The test suite starts 2 Defra nodes, waits for them to connect and then proceeds to update the commonly shared document. We then ensure that the changes on one node are reflected on the other.

fredcarle added feature New feature or request area/testing Related to any test or testing suite area/p2p Related to the p2p networking system action/no-benchmark Skips the action that runs the benchmark. labels Jul 23, 2022

fredcarle added this to the DefraDB v0.3 milestone Jul 23, 2022

fredcarle requested a review from a team July 23, 2022 07:49

fredcarle self-assigned this Jul 23, 2022

fredcarle force-pushed the fredcarle/test/I564-p2p-integration branch 2 times, most recently from 66ddb95 to e80a384 Compare July 23, 2022 08:14

jsimnz requested changes Jul 23, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/test/I564-p2p-integration branch 2 times, most recently from 7b2d205 to 3ac4e17 Compare July 25, 2022 04:50

jsimnz reviewed Jul 25, 2022

View reviewed changes

jsimnz approved these changes Jul 25, 2022

View reviewed changes

shahzadlone requested changes Jul 25, 2022

View reviewed changes