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

Incorporated integration test code from Datahike #16

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

yflim
Copy link

@yflim yflim commented Mar 31, 2022

Move current integration tests for postgres from Datahike to datahike-jdbc. More generally incorporate that test code for all backends in datahike-jdbc integration tests. See Datahike issue 467.

@yflim yflim requested a review from kordano March 31, 2022 04:43
@whilo
Copy link
Member

whilo commented Dec 25, 2022

This looks good to me, but it is not clear to me why this integration test is necessary if konserve-jdbc already tests all the backends with compliance tests. The konserve abstraction is supposed to be not leaky and its compliance test is supposed to check that. The only thing that would be interesting from an integration test here would be performance benchmarks for Datahike, but again, this mostly depends on konserve benchmarks.

@jsmassa
Copy link
Contributor

jsmassa commented Jan 18, 2023

There needs to be some tests to make sure that the interfaces are properly implemented.

It is debatable though if we need tests for multiple configurations. I cannot see a reason why configurations that work in konserve-jdbc wouldn't work here as well.
Thanks to @TimoKramer Datahike has its own integration test namespace included now, so that one could simply be used to check the core functionality for one config and that should be it.
Testing multiple configurations only makes sense as a kind of documentation.

@whilo
Copy link
Member

whilo commented Jan 18, 2023

Yes, I agree with having integration tests, but not necessarily having to test all the different SQL backends. In principle it is ok to test more than necessary, as long as it does not slow down the feedback loop considerably and, more importantly, blows up the code bases. Ideally this integration test should be a compliance test in Datahike that can be just invoked here with one or multiple SQL configurations. Maybe we should converge on the same terminology for compliance and integration test.

@jsmassa
Copy link
Contributor

jsmassa commented Jan 18, 2023

Ideally this integration test should be a compliance test in Datahike that can be just invoked here with one or multiple SQL configurations. Maybe we should converge on the same terminology for compliance and integration test.

What's your understanding of compliance and integration tests here? For me the tests here are compliance tests and as I said, there are already some in datahike itself here that can be used. Or what is it that you would like to have ideally?

@whilo
Copy link
Member

whilo commented Jan 18, 2023

Yes, that is fine. We just need some compliance/integration test to call for different Datahike backends without having to reimplement all kinds of combinations. We agree on this and you are on top of things. Thanks for tackling this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants