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

Refactor generate_queries devops case to use errs #77

Merged

Conversation

RobAtticus
Copy link
Member

Previously the devops use case generation code used a call to
log.Fatalf when something went wrong. This makes it awkward to test
error conditions when generating queries from other packages, since
we need a way to (a) replace the unexported call to log.Fatalf and
(b) prevent the runtime from actually quitting.

It is better for the library to actually return errors on calls
that can fail, rather than either fataling or panicking. Now other
packages can handle the errors themselves and also test error
conditions in their packages as well.

This refactor was pruned a bit to bubble the 'panic' up one level
for now. When the actual generation code encounters the error
during normal execution, it will panic. But these are easier to
test for and don't require adding hooks to replace the 'fatal'
path in the original package.

@codecov-io
Copy link

Codecov Report

Merging #77 into master will decrease coverage by 0.31%.
The diff coverage is 49.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   57.13%   56.82%   -0.32%     
==========================================
  Files          76       76              
  Lines        3719     3757      +38     
==========================================
+ Hits         2125     2135      +10     
- Misses       1570     1592      +22     
- Partials       24       30       +6
Impacted Files Coverage Δ
...bs_generate_queries/databases/clickhouse/devops.go 10.1% <20%> (+0.16%) ⬆️
...sbs_generate_queries/databases/cassandra/devops.go 25.55% <28.57%> (+0.86%) ⬆️
...d/tsbs_generate_queries/databases/influx/devops.go 32.14% <28.57%> (+0.14%) ⬆️
...d/tsbs_generate_queries/databases/siridb/devops.go 27.58% <28.57%> (+0.66%) ⬆️
...s_generate_queries/databases/timescaledb/devops.go 97.41% <87.5%> (-1.2%) ⬇️
cmd/tsbs_generate_queries/uses/devops/common.go 92.85% <90.47%> (-3.97%) ⬇️

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 3cfaae0...70ddcb7. Read the comment docs.

@@ -40,9 +39,6 @@ const (
LabelHighCPU = "high-cpu"
)

// for ease of testing
Copy link
Contributor

Choose a reason for hiding this comment

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

I must admit, I did find this ironic when I first read it 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah enjoyed a nice chuckle as I removed it

Copy link
Contributor

@antekresic antekresic left a comment

Choose a reason for hiding this comment

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

Thanks for this!

I wanted to do it myself but decided to finish up the current PR that I had in line before doing that.

Previously the devops use case generation code used a call to
log.Fatalf when something went wrong. This makes it awkward to test
error conditions when generating queries from other packages, since
we need a way to (a) replace the unexported call to log.Fatalf and
(b) prevent the runtime from actually quitting.

It is better for the library to actually return errors on calls
that can fail, rather than either fataling or panicking. Now other
packages can handle the errors themselves and also test error
conditions in their packages as well.

This refactor was pruned a bit to bubble the 'panic' up one level
for now. When the actual generation code encounters the error
during normal execution, it will panic. But these are easier to
test for and don't require adding hooks to replace the 'fatal'
path in the original package.
@RobAtticus RobAtticus force-pushed the rrk/remove-fatal-gen-queries-uses branch from 70ddcb7 to 3ba75f6 Compare May 22, 2019 15:33
@RobAtticus RobAtticus merged commit ff6e61a into timescale:master May 22, 2019
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.

3 participants