Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Add P2P tests for Schema Update adding field #1182

Merged
merged 7 commits into from
Mar 16, 2023

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #1181

Description

Adds P2P tests for a Schema Update adding a new field. Also fixes the wait mechanic for the P2P tests, that was previously working accidentally - bug was exposed by the new tests.

Includes two documentation tests for what may be undesirable behaviour, where P2P will not sync a new field value that has been added to both nodes. I do not know if this is a bug or a feature, if it is a bug let me know and I can open a ticket. Tests are TestP2POneToOneReplicatorCreateWithNewFieldSyncsDocsToUpdatedSchemaVersion and TestP2PPeerCreateWithNewFieldSyncsDocsToUpdatedSchemaVersion.

@AndrewSisley AndrewSisley added area/schema Related to the schema system 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 Mar 15, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.5 milestone Mar 15, 2023
@AndrewSisley AndrewSisley requested a review from a team March 15, 2023 20:37
@AndrewSisley AndrewSisley self-assigned this Mar 15, 2023
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #1182 (6844134) into develop (561836d) will increase coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1182      +/-   ##
===========================================
+ Coverage    68.65%   68.76%   +0.11%     
===========================================
  Files          180      180              
  Lines        17046    17046              
===========================================
+ Hits         11703    11722      +19     
+ Misses        4387     4372      -15     
+ Partials       956      952       -4     

see 3 files with indirect coverage changes

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a few suggestions related to "typos"

Indeed the email not syncing even when the schema is updated on both sides is an undesirable behaviour. It sounds like the block sent through the P2P network doesn't have the link to the email field. But it might be something else.

CollectionID: 0,
},
testUtils.SchemaPatch{
// Patch the schema on the node that we will directly create a doc on
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: update a doc?

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) Cheers

  • fix comment

},
testUtils.UpdateDoc{
// Update the existing field on the first node only, and allow the value to sync
// We need to make sure any errors caused by the first update to not break the sync
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: do not break.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers

  • fix comment

TargetNodeID: 1,
},
testUtils.SchemaPatch{
// Patch the schema on the node that we will directly create a doc on
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: update a doc on?

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers

  • fix comment

CollectionID: 0,
},
testUtils.SchemaPatch{
// Patch the schema on the node that we will directly create a doc on
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: update a doc on?

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers

  • fix comment

@AndrewSisley AndrewSisley force-pushed the sisley/test/I1181-p2p-schema-update-tests branch from 5d30037 to 6844134 Compare March 16, 2023 18:09
NodeID: immutable.Some(0),
Doc: `{
"Name": "Shahzad",
"Email": "[email protected]"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol: "imnotyourbuddyguy" :,)

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AndrewSisley AndrewSisley merged commit f01563b into develop Mar 16, 2023
@AndrewSisley AndrewSisley deleted the sisley/test/I1181-p2p-schema-update-tests branch March 16, 2023 18:41
shahzadlone pushed a commit that referenced this pull request Apr 13, 2023
* Fix wait mechanic for peer col subscription

Was only working accidentally with the existing tests, and does not work with tests soon to be added.

* Add peer create tests across schema versions

* Add peer update tests across schema versions

* Add replicator create tests across schema versions

* Add replicator update tests across schema versions
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
* Fix wait mechanic for peer col subscription

Was only working accidentally with the existing tests, and does not work with tests soon to be added.

* Add peer create tests across schema versions

* Add peer update tests across schema versions

* Add replicator create tests across schema versions

* Add replicator update tests across schema versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/p2p Related to the p2p networking system area/schema Related to the schema system area/testing Related to any test or testing suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add P2P tests that cover new fields added to node(s)
3 participants