test: Expand tests for Peer subscribe actions #1287
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The function under test takes a set of schemaIDs, so the tests should be able to set multiple in the same test action - this will allow testing of the function with multiple ids.
Adds tests for subscribing to multiple collections in the same call. This is done in a new sub-directory as we will expand the number of subscription-specific tests shortly and they should not polute or by drowned out by the normal/existing p2p-peer tests.
AndrewSisley
added
area/testing
AndrewSisley
changed the title
AndrewSisley
force-pushed
the
sisley/test/I1281-test-p2p-rm-col
branch
from
13b3963
to
7867a95
Compare
Whilst there is not much difference in terms of behaviour, they are still different and it notable enough to result in a test failure
AndrewSisley
force-pushed
the
sisley/test/I1281-test-p2p-rm-col
branch
2 times, most recently
from
5a4f653
to
870184b
Compare
AndrewSisley
force-pushed
the
sisley/test/I1281-test-p2p-rm-col
branch
from
870184b
to
31bc6c7
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1287 +/- ##
===========================================
+ Coverage 70.26% 70.56% +0.29%
===========================================
Files 184 184
Lines 17798 17817 +19
===========================================
+ Hits 12506 12572 +66
+ Misses 4343 4296 -47
Partials 949 949
|
shahzadlone
reviewed
Apr 5, 2023
net/peer.go
Outdated
| @@ -803,7 +806,10 @@ func (p *Peer) AddP2PCollections(collections []string) error { | |||
| return txn.Commit(p.ctx) | |||
| } | |||
|
|
|||
| // RemoveP2PCollectionTopics adds the collectionID from the pubsup topics | |||
| // RemoveP2PCollectionTopics removes the given collectionIDs from the pubsup topics. | |||
There was a problem hiding this comment.
todo:
Suggested change
| // RemoveP2PCollectionTopics removes the given collectionIDs from the pubsup topics. | |
| // RemoveP2PCollections removes the given collectionIDs from the pubsup topics. |
There was a problem hiding this comment.
Will sort out
- Upadate Doc func name
net/peer.go
Outdated
| @@ -758,7 +758,10 @@ type EvtPubSub struct { | |||
| Peer peer.ID | |||
| } | |||
|
|
|||
| // AddP2PCollectionTopic adds the collectionID to the pubsup topics | |||
| // AddP2PCollectionTopic adds the given collectionIDs to the pubsup topics. | |||
There was a problem hiding this comment.
todo:
Suggested change
| // AddP2PCollectionTopic adds the given collectionIDs to the pubsup topics. | |
| // AddP2PCollections adds the given collectionIDs to the pubsup topics. |
There was a problem hiding this comment.
Will sort out
- Upadate Doc func name
| // The `n.Peer.RemoveP2PCollections(colIDs)` call above is calling some asynchronous functions | ||
| // for the pubsub subscription and those functions can take a bit of time to complete, | ||
| // we need to make sure this has finished before progressing. | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
question: how did you determine that it should be 100 milliseconds?
There was a problem hiding this comment.
It is arbitrary, the same sleep is used for after AddP2PSubscription
fredcarle
approved these changes
Apr 5, 2023
There was a problem hiding this comment.
suggestion: We could probably add a test where we specify no collection in the subscription request and it will add automatically add all collections
There was a problem hiding this comment.
The code does not look like it does that - it would just add nothing? Is a good spot either way though, as it is not tested :)
- Close test gap
There was a problem hiding this comment.
Sorry my mistake. I mixed up with replicators.
shahzadlone
pushed a commit
that referenced
this pull request
Apr 13, 2023
* Expand peer P2PCollection func documentation * Allow framework to test multi col ids for subscribe The function under test takes a set of schemaIDs, so the tests should be able to set multiple in the same test action - this will allow testing of the function with multiple ids. * Add tests for multiple peer subscribe Adds tests for subscribing to multiple collections in the same call. This is done in a new sub-directory as we will expand the number of subscription-specific tests shortly and they should not polute or by drowned out by the normal/existing p2p-peer tests. * Add tests for remove peer subscription * Return empty array instead of default Whilst there is not much difference in terms of behaviour, they are still different and it notable enough to result in a test failure * Add tests for GetAllP2PCollections * Add tests for add subscription with error * Add error tests for remove subscription
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
* Expand peer P2PCollection func documentation * Allow framework to test multi col ids for subscribe The function under test takes a set of schemaIDs, so the tests should be able to set multiple in the same test action - this will allow testing of the function with multiple ids. * Add tests for multiple peer subscribe Adds tests for subscribing to multiple collections in the same call. This is done in a new sub-directory as we will expand the number of subscription-specific tests shortly and they should not polute or by drowned out by the normal/existing p2p-peer tests. * Add tests for remove peer subscription * Return empty array instead of default Whilst there is not much difference in terms of behaviour, they are still different and it notable enough to result in a test failure * Add tests for GetAllP2PCollections * Add tests for add subscription with error * Add error tests for remove subscription
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relevant issue(s)
Resolves #1281
Description
Expands the tests for Peer subscribe actions, adding basic coverage to the previously uncovered Remove and GetAll functions (available through the CLI), and basic error testing for Add and Remove.
One extremely minor production issue found and corrected (commit
Return empty array instead of default). Not sure such an inconsistency would be visible to users, as the only exposure would be via CLI or http.