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.

Sign up for GitHub

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

Jump to bottom

test: Expand commit query tests #541

Merged

AndrewSisley merged 7 commits into develop from sisley/test/I540-commit-select-tests

Jun 21, 2022

Contributor

AndrewSisley commented Jun 20, 2022

Relevant issue(s)

Resolves #540

Description

Expands integration tests for commit queries. Similar to time-travel test PR, this includes a number of tests that are largely there to document behaviour I consider unusual - these tests have comments above the function declaration, when reviewing please feel super free to contradict any of the opinions expressed there.

Notable downside of this PR is that it will increase the pain of changing cid format etc., but until we look at reworking the mechanics of commit-related tests I dont see a way around that.

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, ...

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

Debian Linux

AndrewSisley added area/query area/testing action/no-benchmark labels

AndrewSisley added this to the DefraDB v0.3 milestone

AndrewSisley requested review from jsimnz and a team

June 20, 2022 22:55

AndrewSisley self-assigned this

codecov bot commented Jun 20, 2022 •

edited

Loading

Codecov Report

Merging #541 (e39b64b) into develop (4100dd6) will increase coverage by 0.09%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #541      +/-   ##
===========================================
+ Coverage    54.71%   54.81%   +0.09%     
===========================================
  Files           97       97              
  Lines        13174    13174              
===========================================
+ Hits          7208     7221      +13     
+ Misses        5297     5288       -9     
+ Partials       669      665       -4

Impacted Files	Coverage Δ
query/graphql/planner/dagscan.go	`71.17% <0.00%> (+1.76%)`	⬆️
query/graphql/planner/commit.go	`84.95% <0.00%> (+5.30%)`	⬆️
query/graphql/parser/commit.go	`67.30% <0.00%> (+7.69%)`	⬆️


          Rename existing latest_commit tests

4233cff

AndrewSisley force-pushed the sisley/test/I540-commit-select-tests branch from abe57fd to 4391149 Compare

June 21, 2022 00:19

fredcarle reviewed

View reviewed changes

tests/integration/query/all_commits/simple_test.go Outdated

@@ @@ -16,77 +16,25 @@ import ( @@
               	testUtils "github.com/sourcenetwork/defradb/tests/integration"
               )
-              func TestQueryAllCommitsSingleDAG(t *testing.T) {
+              // This test is for documentation reasons only. I do not see this

Collaborator

fredcarle Jun 20, 2022 •

edited

Loading

suggestion: The part starting with I do not probably shouldn't be there as it's seems more like a note or a discussion point with the team.

Sorry, you will find a repeat of this comment quite often bellow.

Contributor Author

AndrewSisley Jun 21, 2022 •

edited

Loading

I agree :) And will remove them all lol

Remove opinion-sentiment from code-docs

tests/integration/query/all_commits/with_dockey_field_test.go Outdated

+              	executeTestCase(t, test)
+              }
+              // This test is for documentation reasons only. I do not see this

Collaborator

fredcarle Jun 20, 2022

Same as my previous comment for the I do not part.

tests/integration/query/all_commits/with_dockey_field_test.go Outdated

+              	executeTestCase(t, test)
+              }
+              // This test is for documentation reasons only. I do not see this

Collaborator

fredcarle Jun 20, 2022

Same as my previous comment for the I do not part.

tests/integration/query/all_commits/with_dockey_test.go Outdated

+              	executeTestCase(t, test)
+              }
+              // This test is for documentation reasons only. I do not see this

Collaborator

fredcarle Jun 20, 2022

Same as my previous comment for the I do not part.

tests/integration/query/all_commits/with_field_test.go Outdated

+              	testUtils "github.com/sourcenetwork/defradb/tests/integration"
+              )
+              // This test is for documentation reasons only. I do not see this

Collaborator

fredcarle Jun 20, 2022

Same as my previous comment for the I do not part.

tests/integration/query/latest_commits/with_dockey_field_test.go Outdated

+              	executeTestCase(t, test)
+              }
+              // This test is for documentation reasons only. I do not see this

Collaborator

fredcarle Jun 21, 2022

Same as my previous comment for the I do not part.

tests/integration/query/latest_commits/with_field_test.go Outdated

+              	testUtils "github.com/sourcenetwork/defradb/tests/integration"
+              )
+              // This test is for documentation reasons only. I do not see this

Collaborator

fredcarle Jun 21, 2022

Same as my previous comment for the I do not part.

tests/integration/query/latest_commits/with_field_test.go Outdated

+              	executeTestCase(t, test)
+              }
+              // This test is for documentation reasons only. I do not see this

Collaborator

fredcarle Jun 21, 2022

Same as my previous comment for the I do not part.

tests/integration/query/commit/with_cid_test.go

+              				}`,
+              			},
+              		},
+              		ExpectedError: "ipld: could not find",

Collaborator

fredcarle Jun 21, 2022

though: out of scope but we should probably take note to return a better error.

tests/integration/query/latest_commits/with_dockey_field_test.go

+              	executeTestCase(t, test)
+              }
+              func TestQueryLatestCommitsWithDocKeyAndCompositeFieldId(t *testing.T) {

Collaborator

fredcarle Jun 21, 2022

suggestion: maybe it would be helpful to document how C relates to the composite field id.

Contributor Author

AndrewSisley Jun 21, 2022 •

edited

Loading

Will leave as is I think. 'C' is a pretty core part of Defra, and hopefully this test wont live for very long (the fact that 'C' works here is not wonderful and it looks like I have forgotten to mention that on this test.

doc test asserting bad behaviour

AndrewSisley force-pushed the sisley/test/I540-commit-select-tests branch from 4391149 to ba102f7 Compare

June 21, 2022 13:22

AndrewSisley added 6 commits

June 21, 2022 09:24


          Expand latest commits tests

88d36b3


          Rename commit with cid test

d5bf9ef


          Expand commit tests

34536d6


          Rename all commits tests

d2979e7


          Expand all commits tests

a343324


          Reword time-travel test docs

e39b64b

AndrewSisley force-pushed the sisley/test/I540-commit-select-tests branch from ba102f7 to e39b64b Compare

June 21, 2022 13:24

AndrewSisley requested a review from fredcarle

June 21, 2022 13:24

fredcarle approved these changes

View reviewed changes

Collaborator

fredcarle left a comment

LGTM 👍

AndrewSisley merged commit 0126567 into develop

AndrewSisley deleted the sisley/test/I540-commit-select-tests branch

June 21, 2022 15:00

shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request


          test: Expand commit query tests (sourcenetwork#541)

b573263

* Rename existing latest_commit tests

* Expand latest commits tests

* Rename commit with cid test

* Expand commit tests

* Rename all commits tests

* Expand all commits tests

* Reword time-travel test docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action/no-benchmark area/query area/testing