test: Migrate P2P/state tests to new framework #1160
Conversation
AndrewSisley
added
bug
|
praise: Thanks for removing the whole |
AndrewSisley
force-pushed
the
sisley/test/I1153-mv-p2p-core-framework
branch
2 times, most recently
from
bd7ca96
to
07d7e68
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1160 +/- ##
==========================================
Coverage ? 68.58%
==========================================
Files ? 180
Lines ? 17012
Branches ? 0
==========================================
Hits ? 11668
Misses ? 4388
Partials ? 956
|
AndrewSisley
force-pushed
the
sisley/test/I1153-mv-p2p-core-framework
branch
2 times, most recently
from
0a01528
to
0dfbd90
Compare
tests/integration/p2p.go
Outdated
| // All document changes made in the source node will be synced to the target node. | ||
| // Changes made in the target node will not be synced to the source node. |
There was a problem hiding this comment.
suggestion: Changes in the target will be synced if it‘s an update to a document that is on both nodes. So a new document in the source will sync to the target and an update to that document on the target will sync to the source. Maybe the comment should be updated to represent that.
There was a problem hiding this comment.
Cheers Fred - will change. I think we may have a test gap there though (might add a ticket).
- Fix ConfigureReplicator docs
There was a problem hiding this comment.
I added a test and updated the docs, it required a minor rejig of the wait mechanics (PR FIXUP - Document replicator target to source sync).
tests/integration/p2p.go
Outdated
| // Any peers connected to this node in the network will have all document changes synced to | ||
| // them from this node. |
There was a problem hiding this comment.
todo: That is an incorrect description of the behaviour. It should say that any changes to documents within the collection on any network nodes will sync to this node.
There was a problem hiding this comment.
Cheers! I think we may have a test gap here too, as I think I remember being unsure about this in particular and couldn't find any tests that demonstrated this distinction (I think they may all be one-one node tests, where the result appears on all nodes present).
- Fix documentation, maybe add ticket for test gap
There was a problem hiding this comment.
We do have tests for this, I missed them. Docs have been updated
tests/integration/p2p.go
Outdated
|
|
||
| // The `n.Peer.AddP2PCollections(colIDs)` call above is calling some asynchronous functions | ||
| // for the pubsub subscription and those functions can take a bit of time to complete. | ||
| // Since it is possible to get to the create and update functions bellow before the subscription |
There was a problem hiding this comment.
suggestion: Remove bellow as it's no longer accurate.
There was a problem hiding this comment.
My bad, thanks for flagging...
- Fix out of date comment
tests/integration/utils2.go
Outdated
| for _, db := range dbs { | ||
| executeTestCase(ctx, t, collectionNames, testCase, db) |
There was a problem hiding this comment.
suggestion: Since the parameter name is dbt, maybe it would be more appropriate to use dbt here as well instead of db which can be confused with a client.DB type.
There was a problem hiding this comment.
Makes sense, will change
- rename db=> dbt var
| // This document is created in all nodes before the replicator is set up. | ||
| // Updates should be synced across nodes. |
There was a problem hiding this comment.
todo: in node 1 instead of in all nodes.
There was a problem hiding this comment.
😆 fair enough, will change
- tweak comment
tests/integration/p2p.go
Outdated
| if action.NodeID.HasValue() && action.NodeID.Value() == cfg.TargetNodeID { | ||
| targetToSourceEvents += 1 | ||
| } |
There was a problem hiding this comment.
todo: This situation will not result in a sync so we shouldn't wait. Currently it will cause the test to timeout if we try to test that a create on the target will not sync to the source.
There was a problem hiding this comment.
Will test and get back to you. I spent an annoying amount of time getting these if block/waits correct, but that time was also polluted with the bug fixed in this PR and I trust you here more than I trust me.
- Try and remove, if tests fail check in with Fred
There was a problem hiding this comment.
TestP2PWithMultipleDocumentUpdatesPerNode failed on first test run without this line (targetToSourceEvents += 1). I think either you may be mistaken (possibly due to production bug), or there is another bug somewhere in the test framework (perhaps unlikely given the nature of this line/failure).
Leaving as is for now, but it might be worth more of your attention as I am not so sure what the correct P2P behaviour should be, only what it appears to actually be.
There was a problem hiding this comment.
actually that failure makes no sense, as removing that line shouldn't increase the chances of a time out, only increase the chances of checking the results prematurely - likely an unhappy coincidence, and suggests that there may still be some flakiness. Will investigate further before having another look at this line again.
There was a problem hiding this comment.
Looks like failure was just #1154 which I thought had been fixed. Removing this if block - thanks Fred
tests/integration/p2p.go
Outdated
| if action.NodeID.HasValue() && action.NodeID.Value() == cfg.TargetNodeID { | ||
| targetToSourceEvents += 1 | ||
| } |
There was a problem hiding this comment.
todo: This will only be relevant if the document was previously created on both source and target. We should have a way to verify that before committing to wait for a sync otherwise the test will timeout.
There was a problem hiding this comment.
I think we have a test gap here then - will try and sort out both gap and this code in this PR.
- Add test, verify (framework) bug, fix bug
There was a problem hiding this comment.
On revisit, I think you may be incorrect here as there is no way for the document to not exist in all nodes.
If a document didnt exist everywhere prior to replicator configuration, the replicator would sync it upon configuration. I have just added a test for this (PR FIXUP - Add test for replicator syncing exising doc).
There was a problem hiding this comment.
No I'm pretty sure I'm correct :)
A replicator is a one way on create. If node 1 replicates to node 2, new docs on node 1 will be copied to node 2. But new docs on node 2 will not replicate to node 1.
However, updating a documents on node 2 that was either seeded to both nodes or created on node 1 will also result in that document being updated on node 1. But a document updated on node 2 that was created on node 2 will not be updated on node 1.
There was a problem hiding this comment.
😁 Of course lol, was only thinking for creates on the source. Cheers
- Add test, probably make changes to framework
AndrewSisley
force-pushed
the
sisley/test/I1153-mv-p2p-core-framework
branch
from
a146614
to
f053be0
Compare
AndrewSisley
force-pushed
the
sisley/test/I1153-mv-p2p-core-framework
branch
from
f8f8108
to
4822c34
Compare
Flagged by go test -race
It does not work with errors.Is, which is kind of important and breaks P2Ps transaction rentry code
Is an unnessecary complication that makes future changes harder
databaseType will be used more in later commits. It can stay internal and hidden behind the `Name()` function for the explain tests for now.
We need to be able to initialize database instances from within an action, but they should still be using the correct datastore type! We also require 1-many database instances for a given test (one per node). This commit enables that, and creates a single 'normal' node if no nodes have been explicitly requested. The explain utils starts to get in the way a little bit here, and will be properly separated shortly in another commit.
databaseInfo and a handful of utils functions are now only used by the explain tests and represent an annoying code complication and oddity for the main test suite. This commit removes what it can, and moves the rest into the explain test utils.
Subscriptions, transactional queries and the change detector are currently out of scope, although I see no reason as to why they could not be made to work later. Actually using these new actions has been split out to another commit in order to keep this on focused, it may however be handy to have a look at the two together to get an idea as to what it looks like to use it.
Does not change the nature of the tests, although I do remember deleting a single test that acted as a sanity check for the old framework. Does not make use of localised utils.go files in order to hide the schema - whilst they are consistent across a directory I am currently prefering the explicitness here given how complex these tests are. Do let me know though.
Behaviour does not exist, no need to wait.
* Remove replicator race condition Flagged by go test -race * Remove Defra-IM txn error fanciness It does not work with errors.Is, which is kind of important and breaks P2Ps transaction rentry code * Remove dbInfo.Rootstore func Is an unnessecary complication that makes future changes harder * Extract datastore strings to consts databaseType will be used more in later commits. It can stay internal and hidden behind the `Name()` function for the explain tests for now. * Move control of db instance to action handler We need to be able to initialize database instances from within an action, but they should still be using the correct datastore type! We also require 1-many database instances for a given test (one per node). This commit enables that, and creates a single 'normal' node if no nodes have been explicitly requested. The explain utils starts to get in the way a little bit here, and will be properly separated shortly in another commit. * Remove databaseInfo from main test utils databaseInfo and a handful of utils functions are now only used by the explain tests and represent an annoying code complication and oddity for the main test suite. This commit removes what it can, and moves the rest into the explain test utils. * Add P2P capacity to core test framework Subscriptions, transactional queries and the change detector are currently out of scope, although I see no reason as to why they could not be made to work later. Actually using these new actions has been split out to another commit in order to keep this on focused, it may however be handy to have a look at the two together to get an idea as to what it looks like to use it. * Convert existing P2P tests to new system Does not change the nature of the tests, although I do remember deleting a single test that acted as a sanity check for the old framework. Does not make use of localised utils.go files in order to hide the schema - whilst they are consistent across a directory I am currently prefering the explicitness here given how complex these tests are. Do let me know though. * Remove old P2P/state test framework
* Remove replicator race condition Flagged by go test -race * Remove Defra-IM txn error fanciness It does not work with errors.Is, which is kind of important and breaks P2Ps transaction rentry code * Remove dbInfo.Rootstore func Is an unnessecary complication that makes future changes harder * Extract datastore strings to consts databaseType will be used more in later commits. It can stay internal and hidden behind the `Name()` function for the explain tests for now. * Move control of db instance to action handler We need to be able to initialize database instances from within an action, but they should still be using the correct datastore type! We also require 1-many database instances for a given test (one per node). This commit enables that, and creates a single 'normal' node if no nodes have been explicitly requested. The explain utils starts to get in the way a little bit here, and will be properly separated shortly in another commit. * Remove databaseInfo from main test utils databaseInfo and a handful of utils functions are now only used by the explain tests and represent an annoying code complication and oddity for the main test suite. This commit removes what it can, and moves the rest into the explain test utils. * Add P2P capacity to core test framework Subscriptions, transactional queries and the change detector are currently out of scope, although I see no reason as to why they could not be made to work later. Actually using these new actions has been split out to another commit in order to keep this on focused, it may however be handy to have a look at the two together to get an idea as to what it looks like to use it. * Convert existing P2P tests to new system Does not change the nature of the tests, although I do remember deleting a single test that acted as a sanity check for the old framework. Does not make use of localised utils.go files in order to hide the schema - whilst they are consistent across a directory I am currently prefering the explicitness here given how complex these tests are. Do let me know though. * Remove old P2P/state test framework
Relevant issue(s)
Resolves #1153
Description
I need to be able to do some quite complicated testing with P2P and Schema Updates, and so have merged the P2P/state tests into the primary integration test framework. I think it is quite a nice change regardless of the immediate need too.
As quite a lot has changed in this PR, and a lot moved - I think similar to the previous integration test rework PR the code that has been moved should be reviewed as if it was brand new - it is a good opportunity to further clean it up, and it is probably a waste of everyone's energy to try differentiate between what is new/re-written and what has just been moved.
The additional testing to be done as part of Schema Updates is not in scope for this PR. The tests remain the same, minus a single test that was removed (it was a sanity check for the old framework).
Strongly recommend reviewing commit by commit, and reading the full commit message. This got a bit fiddly at points and the commits are perhaps not as clean/clear cut as I'd like, but still much better than just trying to read the whole thing in one go - sorry about that.
Also fixes a previously undetected P2P race condition between node Start and SetReplicator. And temporarily removes some error magic that does not appear to quite be working (was not caught previously as P2P tests only ran on badger).
Will be running the test action a bunch of times whilst in review to see if there is any flakiness left - I am happy tolerating some if found, as the current framework is slightly flaky and I see fully solving that as out of scope, but I definitely do not wish to make it worse.I ran them 20 or so times (CI) without a failure, I think all flakiness may finally be fixed.#1154 is still an issue, got a single failure out of 50 runs. Will not fix in this PR.