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

Add unit tests to query generation functions for various databases #76

Merged

Conversation

antekresic
Copy link
Contributor

Covering query generation functions for Influx, ClickHouse and SiriDB
databases. Tests are covering basic pre-generated outputs and provide
visual sanity checks. More robust tests are left as a future task.

@antekresic antekresic self-assigned this May 21, 2019
@codecov-io
Copy link

codecov-io commented May 21, 2019

Codecov Report

Merging #76 into master will increase coverage by 7.58%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #76      +/-   ##
=========================================
+ Coverage   57.32%   64.9%   +7.58%     
=========================================
  Files          77      77              
  Lines        3810    3810              
=========================================
+ Hits         2184    2473     +289     
+ Misses       1595    1309     -286     
+ Partials       31      28       -3
Impacted Files Coverage Δ
...d/tsbs_generate_queries/databases/influx/devops.go 100% <0%> (+67.85%) ⬆️
...d/tsbs_generate_queries/databases/siridb/devops.go 100% <0%> (+72.41%) ⬆️
...bs_generate_queries/databases/clickhouse/devops.go 100% <0%> (+89.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a985875...c1bc78a. Read the comment docs.

rand.Seed(123) // Setting seed for testing purposes.

for _, c := range cases {
t.Run(c.desc, func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

wow, i never knew about this but it's exactly what i've wanted and (poorly) re-implemented all over the place.

if s != c.failMsg {
t.Fatalf("incorrect fail message: got %s, want %s", s, c.failMsg)
}
runtime.Goexit()
Copy link
Member

Choose a reason for hiding this comment

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

moot point if we go with my other pr (#77), but is this necessary? we don't have to have the same semantics of log.Fatalf (i.e. killing the process). If we take this out, we can reduce the amount of defer/waitgroup also, making this an easier func to understand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree but if we don't kill the process, fatal will be called multiple times in a test case and thats something that the existing function stops from happening. That means tests would fail and I would have to have multiple fatalMsgs to check each one.

I know it looks over-engineered but I didn't find a better way of replicating the current behavior in order that the tests reflect real-world usage.

@antekresic
Copy link
Contributor Author

@RobAtticus updated the tests based on your feedback. Waiting with further changes until PR #77 is merged to master.

@antekresic antekresic force-pushed the maint/generate-query-unit-tests branch from ef57f51 to 00454c0 Compare May 23, 2019 09:23
@antekresic antekresic requested a review from RobAtticus May 23, 2019 09:24
@antekresic
Copy link
Contributor Author

@RobAtticus I rebased the PR with your latest changes and fixed them up to reflect them.

Could you take another look at it?

Thanks.

Copy link
Member

@RobAtticus RobAtticus left a comment

Choose a reason for hiding this comment

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

Couple of small things, but then you can merge

Covering query generation functions for Influx, ClickHouse and SiriDB
databases. Tests are covering basic pre-generated outputs and provide
visual sanity checks. More robust tests are left as a future task.
@antekresic antekresic force-pushed the maint/generate-query-unit-tests branch from 00454c0 to c1bc78a Compare May 24, 2019 07:03
@antekresic antekresic merged commit f7b8830 into timescale:master May 24, 2019
@antekresic antekresic deleted the maint/generate-query-unit-tests branch May 24, 2019 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants