fix: Rework the P2P integration tests

[AndrewSisley]

Relevant issue(s)

Resolves #985

Description

I started changing stuff as the Replicator tests permitted tests to pass when they shouldn't (first commit). This PR then goes on to expand the replicator tests. Then, as my understanding grew I reworked the rest for the reasons laid out in the message body of the 3rd commit.

I am planning to expand the replicator and peer tests further (create for peer, update for replicator, and tests that configure both). I am quite new to this part of the codebase though, so it is very possible that some of that will not be possible due to me misunderstanding the desired behaviour (will probs badger Fred here). This expansion may take place in this PR if no one gets back to me before I resume work tomorrow - do let me know your thoughts in the meantime though :) New tests have been included

No production bugs have been found yet. Probable production bug found: #1000
Another prod bug found: #1031 - Waiting on this to be merged before finalizing the test code.

[codecov]

Codecov Report

Merging #989 (8b1f0d4) into develop (db76f2d) will increase coverage by 0.28%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #989      +/-   ##
===========================================
+ Coverage    57.74%   58.02%   +0.28%     
===========================================
  Files          174      174              
  Lines        19563    19563              
===========================================
+ Hits         11297    11352      +55     
+ Misses        7265     7215      -50     
+ Partials      1001      996       -5     

Impacted Files	Coverage Δ
node/node.go	66.38% <ø> (ø)
net/pb/net.pb.go	15.82% <0.00%> (+0.16%)	⬆️
datastore/badger/v3/datastore.go	39.12% <0.00%> (+0.32%)	⬆️
net/server.go	57.08% <0.00%> (+1.14%)	⬆️
net/process.go	77.61% <0.00%> (+2.98%)	⬆️
datastore/blockstore.go	54.94% <0.00%> (+3.29%)	⬆️
net/peer.go	50.47% <0.00%> (+6.45%)	⬆️
core/crdt/lwwreg.go	75.86% <0.00%> (+6.89%)	⬆️
net/client.go	85.36% <0.00%> (+7.31%)	⬆️
net/dialer.go	56.25% <0.00%> (+12.50%)	⬆️

[fredcarle]

I'm happy to see that you dived into the P2P related code. I wasn't expecting this much change on the test utility though haha. There are a few things you changed that I think shouldn't have been changed. The peer test was done in that way because it allowed for testing the pubsub process in a deterministic fashion. For each updates, the test would wait for the receiving peer to acknowledge reception and then the result would be asserted. Eventual consistency is the outcome of processing a log but not the intent of the test.

The pubsub network can only update documents that where at first created exactly the same on both nodes. Hence why the check is on a specific document and a getAllDocuments type function isn't applicable (although it could be used for the replicator tests). Create wont be possible for pubsub related tests. Update is definitely possible for the replicators though.

The key points to keep in mind is that pubsub configuration happens during the node setup while replicators are added to an already running node. Peers on the pubsub network subscribe to a specific document's updates while replicators subscribe to all actions on a collection (or any number of collections).

If you want me to get on a call to discuss this with you tomorrow feel free to reach out.

[AndrewSisley]

Eventual consistency is the outcome of processing a log but not the intent of the test.

No, but it is ultimately what we care about, and far more intuitive to read IMO (as well as being more consistent with all our other tests).

Create wont be possible for pubsub related tests

Correction - created records should not be synced - this is untested and needs to be tested, and previously could not be tested. We also need to test that newly created records should also not break the peer system.

The key points to keep in mind is that pubsub configuration happens during the node setup while replicators are added to an already running node. Peers on the pubsub network subscribe to a specific document's updates while replicators subscribe to all actions on a collection (or any number of collections).

I understand this, and there are large test holes described in the description that are now possible to fill. It sounds like it would be prudent to hold off on further review until I add those tests as they will do a better job at describing these holes, as well as the duplication/conflicts present in the old system.

[fredcarle]

No, but it is ultimately what we care about, and far more intuitive to read IMO (as well as being more consistent with all our other tests).

The way you changed the test makes it impossible to test that the peer got the update as the test will pass even if the value stays the same.

[AndrewSisley]

No, but it is ultimately what we care about, and far more intuitive to read IMO (as well as being more consistent with all our other tests).

The way you changed the test makes it impossible to test that the peer got the update as the test will pass even if the value stays the same.

I dont follow. You are talking about on update? The tests show that the values have been synced, and are the same across both systems (something the prior version was unable to assert). It does not check the commit history, but that is easy enough to add if we want, and I would suggest that is highly secondary to the fact that they both have the same head value (especially given that the history is somewhat required to achieve that state, or the test would be flacky as it would only pass ~half of the time).

[fredcarle]

I dont follow. You are talking about on update? The tests show that the values have been synced, and are the same across both systems (something the prior version was unable to assert).

You are asserting with anyOf which will pass regardless of either value. The previous version was able to assert that the update was received and that the value was as expected for the given update. They are both different types of test and don't provide the same guarantees. We can keep what you added but I think the original one should be kept as well. That test was very useful during development.

[AndrewSisley]

I dont follow. You are talking about on update? The tests show that the values have been synced, and are the same across both systems (something the prior version was unable to assert).

You are asserting with anyOf which will pass regardless of either value. The previous version was able to assert that the update was received and that the value was as expected for the given update. They are both different types of test and don't provide the same guarantees. We can keep what you added but I think the original one should be kept as well. That test was very useful during development.

This is incorrect. AnyOf asserts that they have any of the given values, but that the values are the same across all nodes. This is stated in the docs. It is a very important distinction, and very useful public documentation IMO. The previous tests did not describe this behaviour at all, and it is very important that it is.

[AndrewSisley]

Actions discussed outside of github:

• Preserve original tests - I will likely move the new code to a different directory and revert the original. I still disagree that the old are useful alongside the new, and I disagree that the old are integration tests (as they dont test publicly useful behaviour). But we did agree to do this before Christmas.

[fredcarle]

This is great. Thank for engaging with the suggestions :)

[AndrewSisley]

This is great. Thank for engaging with the suggestions :)

Thank you for suggesting them (and all the other related work around this) :)

Issues addressed and corrected