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 cid (time-travel) query tests #539

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

AndrewSisley
Copy link
Contributor

@AndrewSisley AndrewSisley commented Jun 20, 2022

Relevant issue(s)

Resolves #526

Description

Adds integration tests for cid (time-travel) queries. A many of these tests document undesired features - when reviewing keep an eye out for test-function level documentation and feel very free to correct/object to my opinions expressed in those.

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 AndrewSisley added area/query Related to the query component ci/build This is issue is about the build or CI system, and the administration of it. action/no-benchmark Skips the action that runs the benchmark. labels Jun 20, 2022
@AndrewSisley AndrewSisley requested review from jsimnz and a team June 20, 2022 19:32
@AndrewSisley AndrewSisley self-assigned this Jun 20, 2022
@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #539 (e71e6ec) into develop (bb08df0) will increase coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #539      +/-   ##
===========================================
+ Coverage    54.42%   54.71%   +0.28%     
===========================================
  Files           97       97              
  Lines        13174    13174              
===========================================
+ Hits          7170     7208      +38     
+ Misses        5325     5297      -28     
+ Partials       679      669      -10     
Impacted Files Coverage Δ
query/graphql/parser/query.go 83.98% <0.00%> (+2.11%) ⬆️
query/graphql/planner/select.go 79.10% <0.00%> (+3.43%) ⬆️
query/graphql/planner/scan.go 87.38% <0.00%> (+5.40%) ⬆️
db/fetcher/versioned.go 55.72% <0.00%> (+6.25%) ⬆️

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

Very minor suggestions, but straightforward additions, so this is an easy one 👍

Comment on lines +46 to +68
func TestQuerySimpleWithUnknownCidAndInvalidDocKey(t *testing.T) {
test := testUtils.QueryTestCase{
Description: "Simple query with unknown cid and invalid dockey",
Query: `query {
users (
cid: "bafybeid57gpbwi4i6bg7g357vwwyzsmr4bjo22rmhoxrwqvdxlqxcgaqvu",
dockey: "invalid docKey"
) {
Name
}
}`,
Docs: map[int][]string{
0: {
`{
"Name": "John",
"Age": 21
}`,
},
},
ExpectedError: "failed to get block in blockstore: ipld: could not find",
}

executeTestCase(t, test)
Copy link
Member

Choose a reason for hiding this comment

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

suggestion(non-blocking): this can broken into two tests, invalid doc key, and non-existing CID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually doesnt care about the dockey format - there is a test that shows this, but the error is just that it couldn't find the cid (TestQuerySimpleWithUnknownCidAndInvalidDocKey)


func TestQuerySimpleWithCidAndDocKey(t *testing.T) {
test := testUtils.QueryTestCase{
Description: "Simple query with cid and dockey",
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: All three of these tests have the same description. Should add more context, so when it errors out and dumps the description to the terminal it's more meaningful.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Jun 20, 2022

Choose a reason for hiding this comment

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

My bad - I though I tweaked all of those. Will change

  • fix descriptions

// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package simple
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Add a test case that has multiple updates, right now the most updates being applied is 1.

Copy link
Member

Choose a reason for hiding this comment

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

future work can be done to add concurrent updates, but this is far out of scope of this PR as the testing framework doesn't support concurrent updates (causing a forked/multi head situation).

Copy link
Contributor Author

@AndrewSisley AndrewSisley Jun 20, 2022

Choose a reason for hiding this comment

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

suggestion: Add a test case that has multiple updates, right now the most updates being applied is 1.

I was tempted to add that just to test getting a 'middle' cid, but otherwise I struggled to see how it could break that way. I guess I'll add one lol

  • add 'middle' cid test

Copy link
Member

Choose a reason for hiding this comment

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

Theres prob a number of steps the code skips if there is only the root version and 1 extra version, as it doesn't have to work as hard to iterate over all the versions and reconstruct as much.

// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package one_to_many
Copy link
Member

Choose a reason for hiding this comment

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

thought: As mentioned on discord, I don't think these kinds of queries should be supported yet, as the return document will violate the "Time travel" mechanic since the root document will be the target version (cid) but the sub document won't be.

@AndrewSisley AndrewSisley force-pushed the sisley/test/I526-commit-select-tests branch from 95ce7cf to e71e6ec Compare June 20, 2022 20:24
@AndrewSisley AndrewSisley requested a review from jsimnz June 20, 2022 20:24
@AndrewSisley AndrewSisley merged commit 7c69391 into develop Jun 20, 2022
@AndrewSisley AndrewSisley deleted the sisley/test/I526-commit-select-tests branch June 20, 2022 20:30
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
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/query Related to the query component ci/build This is issue is about the build or CI system, and the administration of it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time travel queries have no integration tests
2 participants