-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
added tests for clickhouse database #532
Conversation
Pull Request Test Coverage Report for Build 272
💛 - Coveralls |
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.
Thanks for the PR!
database/clickhouse/clickhouse.go
Outdated
config *Config | ||
conn *sql.DB | ||
config *Config | ||
isLocked bool |
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.
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.
thnx, used
t.Fatal(err) | ||
} | ||
|
||
if err := d.(*ClickHouse).ensureVersionTable(); err != nil { |
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.
Why is ensureVersionTable()
called twice? Does it need to be called at all?
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.
yes, you right. I removed it, thanks
@@ -0,0 +1,184 @@ | |||
package clickhouse |
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.
Can this be clickhouse_test
?
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.
this test included in the package clickhouse, not clichouse_test. Other databases tests included in databases packages too
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 didn't see any usage of unexported identifiers, but could have missed some. If we don't need to use unexported identifiers in tests, let's use a different package name for the tests and import the clickhouse
package.
Other databases tests included in databases packages too
Some db tests test unexported methods. We should really split our tests into internal and external tests...
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 understood. Thanks, fixed
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.
Thanks for address the PR feedback!
Hi!
I want added tests for clickhouse database