Add unit tests to increase code coverage for query generation #72
Conversation
Codecov Report
@@ Coverage Diff @@
## master #72 +/- ##
==========================================
+ Coverage 53.93% 57.13% +3.19%
==========================================
Files 76 76
Lines 3719 3719
==========================================
+ Hits 2006 2125 +119
+ Misses 1690 1570 -120
- Partials 23 24 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #72 +/- ##
==========================================
+ Coverage 53.93% 57.13% +3.19%
==========================================
Files 76 76
Lines 3719 3719
==========================================
+ Hits 2006 2125 +119
+ Misses 1690 1570 -120
- Partials 23 24 +1
Continue to review full report at Codecov.
|
| expectedHumanLabel: "TimescaleDB last row per host", | ||
| expectedHumanDesc: "TimescaleDB last row per host", | ||
| expectedHypertable: "cpu", | ||
| expectedSQLQuery: "SELECT DISTINCT ON (hostname) * FROM cpu ORDER BY hostname, time DESC", |
There was a problem hiding this comment.
I noticed that the LastPointPerHost queries do not depend on the qi arugment of the function, solely on the state of the Devops struct. They use Sprintf with no args, which can be removed.
Also, I'm not sure but since the query is a complete string, can we keep it as a constant in devops.go and reference the same constant here in the tests?
There was a problem hiding this comment.
And with queries that are not constant but have specific parts that are filled in, I suggest that we keep the fixed parts as templates in constant, and then format them with the generated values.
This way the tests will be a lot shorter and all the queries will be in one place in one file for a quick review
There was a problem hiding this comment.
My reasoning was keeping the same style of test for all the query generation functions. Thats why I'm testing a fixed string (which could change to a dynamic one in the future).
As for making the tests shorter by reusing the templates, I guess you would have to basically rewrite all the logic in the function by reusing the same functions used to fill out the template and the test would be mirroring the function in test. I'm not fond of that approach since you are just writing the same logic twice and checking that it generates the same output. I decided to go this way so we can at least see the whole query visually in the test to check for sanity.
I, also, wanted to make a point of not changing the current code until we have some test coverage. After that we can introduce some refactoring at which point we can refactor the tests if there is a need for it.
There was a problem hiding this comment.
Yeah I do like the approach for now of having the whole query, even if it is a bit cluttered. I think @blagojts 's suggestions for that function are good ones for a follow up PR (since as @antekresic mentioned, we will then have tests to cover it in case the refactor goes wrong). Those changes being making the strings consts and not use Sprintf for no reason.
There was a problem hiding this comment.
Couple of small changes, but overall I like the approach.
Additionally consider adding an extended description to the commit with something like
For now the tests are mainly matching the output against pre-generated/known
outputs to ensure we have some coverage. A more robust checking, e.g., making
sure the semantics of the query are actually correct, is a future task.
| expectedHumanLabel: "TimescaleDB last row per host", | ||
| expectedHumanDesc: "TimescaleDB last row per host", | ||
| expectedHypertable: "cpu", | ||
| expectedSQLQuery: "SELECT DISTINCT ON (hostname) * FROM cpu ORDER BY hostname, time DESC", |
There was a problem hiding this comment.
Yeah I do like the approach for now of having the whole query, even if it is a bit cluttered. I think @blagojts 's suggestions for that function are good ones for a follow up PR (since as @antekresic mentioned, we will then have tests to cover it in case the refactor goes wrong). Those changes being making the strings consts and not use Sprintf for no reason.
c8d8859 to
bd7112f
Compare
|
Thanks for the feedback @blagojts and @RobAtticus . I incorporated your suggestions and force pushed the commit with matching extended description. Let me know if there is anything else blocking us from merging this. |
bd7112f to
be70914
Compare
For now the tests are mainly matching the output against pre-generated/known outputs to ensure we have some coverage. A more robust checking, e.g., making sure the semantics of the query are actually correct, is a future task.
be70914 to
fc4ba15
Compare
|
@RobAtticus fixed all the durations. Should be good to go now. |
Making a small PR for unit tests to give people a chance to review and comment on testing style.
Once this one gets approved and merged, expect a bigger PR covering more code using the same style.