-
Notifications
You must be signed in to change notification settings - Fork 328
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
refactor: move database client to tests/common #4057
refactor: move database client to tests/common #4057
Conversation
@@ -61,7 +61,7 @@ url = "2.3" | |||
|
|||
[dev-dependencies] | |||
chrono.workspace = true | |||
client = { workspace = true, features = ["testing"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another question about this: because we already have testing
feature in client
, is it really necessary to move database.rs
out of client
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and we do have only one testing function in client for which we can't remove testing
from client
at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we can move this struct to client
crate behind the testing
feature.
IIRC previously it's in the client
crate without testing
feature in control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can close this one, revert #3820, and create a new patch to feature-gate database.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. This is possible. Would you follow these changes?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4057 +/- ##
==========================================
- Coverage 85.41% 85.10% -0.31%
==========================================
Files 985 985
Lines 170616 170616
==========================================
- Hits 145724 145205 -519
- Misses 24892 25411 +519 |
I'm manually reverting this. The related files changed a bit . |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
Resolves dependencies issue introduced in #3820
What's changed and what's your intention?
This patch extract
database.rs
to standalone module to avoid introducing whole dependency tree of tests-integration.Also it removes dependency from
benchmarks
which is obsolete.Checklist