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 test datastore selection support #88

Merged
merged 3 commits into from
Jan 19, 2022

Conversation

AndrewSisley
Copy link
Contributor

Closes #87

Change driven by recent bugs in development/production (fixes for those still outstanding cherry-picked in).

Hopefully we can add more stores to this, including file-based badger. Still seems quick to run go test ./... on my machine but we can change the defaults if/when it becomes an issue.

@AndrewSisley AndrewSisley added the area/testing Related to any test or testing suite label Dec 9, 2021
@AndrewSisley AndrewSisley self-assigned this Dec 9, 2021
@AndrewSisley AndrewSisley linked an issue Dec 9, 2021 that may be closed by this pull request
db/tests/utils.go Outdated Show resolved Hide resolved
db/tests/utils.go Outdated Show resolved Hide resolved
@jsimnz
Copy link
Member

jsimnz commented Dec 13, 2021

Added my review. But at the same time I wanted to ask a bigger question, that we touched on during our last eng call. Do we want this multi datastore backend test method implemented programmatically, as its done here, or via tooling with a simpler implementation, that then uses Make or scripting to run the tests against multiple DBs.

@AndrewSisley
Copy link
Contributor Author

Cheers John - will go through the line-specific items when I can.

RE: loop vs make/aliases - I'm not keen on putting much in make-type files if we can avoid it, and I do like that it runs both by default when running go test ./... (even if we could move that to make/alias files) - a solid default will help stop devs from leaking bugs.

@AndrewSisley AndrewSisley force-pushed the sisley/test/I87-add-test-datastore-selection branch from 77d80c2 to 8d68075 Compare December 16, 2021 15:37
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.

Lets just quickly add Badger file store, incase theres some weird edge cases there.

}, nil
}

func getDatabases() ([]databaseInfo, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to include Badger disk based DB as well as the in-memory version. Certainly some code paths internal to badger we won't hit if we only work with the Memory version.

I can't see how those extra codepaths would have any effects on the Defra codebase and tests, but if its easy to add to give some piece of mind, why not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do, but it would make the tests dependent on the file system (which maybe is a good thing for a Db project lol...), might have implications for the CI system?

@AndrewSisley AndrewSisley force-pushed the sisley/test/I87-add-test-datastore-selection branch 2 times, most recently from 59f7400 to d145736 Compare January 19, 2022 15:16
@AndrewSisley AndrewSisley force-pushed the sisley/test/I87-add-test-datastore-selection branch from d145736 to 716102a Compare January 19, 2022 16:00
@codecov
Copy link

codecov bot commented Jan 19, 2022

Codecov Report

Merging #88 (3609b6f) into develop (8de4435) will increase coverage by 0.14%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #88      +/-   ##
===========================================
+ Coverage    54.03%   54.17%   +0.14%     
===========================================
  Files           35       35              
  Lines         3633     3629       -4     
===========================================
+ Hits          1963     1966       +3     
+ Misses        1441     1435       -6     
+ Partials       229      228       -1     
Impacted Files Coverage Δ
db/query.go 0.00% <0.00%> (ø)
db/txn.go 61.53% <0.00%> (+5.39%) ⬆️
query/graphql/schema/relations.go 56.18% <0.00%> (+1.54%) ⬆️

@AndrewSisley
Copy link
Contributor Author

Removed badger file-system from this branch, opening new issue #127 and new branch/review sisley/test/I127-test-against-file-system

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.

LGTM

@AndrewSisley AndrewSisley force-pushed the sisley/test/I87-add-test-datastore-selection branch from 716102a to dc5037f Compare January 19, 2022 21:54
@AndrewSisley AndrewSisley force-pushed the sisley/test/I87-add-test-datastore-selection branch from dc5037f to 3609b6f Compare January 19, 2022 21:55
@AndrewSisley AndrewSisley merged commit 746aec2 into develop Jan 19, 2022
@AndrewSisley AndrewSisley deleted the sisley/test/I87-add-test-datastore-selection branch January 19, 2022 21:57
@orpheuslummis orpheuslummis mentioned this pull request Jan 29, 2022
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
* Move discard to after error check

* Fix if-else

ok is always false here

* Add support for running tests against multiple ds types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Related to any test or testing suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add means to select datastore for db/tests
2 participants