incorporation of test improvements from parallel branch #959
Conversation
Updates to include improvements from "merged key dimension table" branch (`krivard/v4-rpp-mergeddim-leftjoin`), specifically at commit hash `fbf878e`. Changes include: - unit/integration testing refactorization and other improvements. - percona dbms now used in db docker image, plus changes for resulting compatibility issues. - db schema names are specified in ddl files. - removed of obsolete index hint guessing method. - updated comments. This changeset is essentially just a port of the excellent work @krivard did to refactor and otherwise improve the test architecture, as she applied it to branch mentioned above. Most of the files were simply copied over from the other branch to this one to create this PR; I only really made edits to these files: (and most edits were to strip out "mergedkey" stuff) - src/ddl/v4_schema.sql - src/acquisition/covidcast/database.py - src/acquisition/covidcast/test_utils.py - integrations/acquisition/covidcast/test_covidcast_meta_caching.py - integrations/server/covidcast/test_covidcast_meta.py
| `value`, `stderr`, `sample_size`, | ||
| `issue`, `lag`, `missing_value`, | ||
| `missing_stderr`,`missing_sample_size`) | ||
| VALUES | ||
| (%d, %d, %d, "%s", %d, 123, | ||
| %d, 0, 0, %d, 0, %d, %d, %d) | ||
| (%d, %d, %d, |
There was a problem hiding this comment.
Do we have code standards set for line length and how to format things like this? Seems to change from file to file and it would be nice to have a unified truth for the project.
There was a problem hiding this comment.
Not in this repo, and we should! Outside the scope of this PR but I made an issue for it.
There was a problem hiding this comment.
we dont have any hard rules. generally, imnsho, the format should be something that "looks clean" (or at least is not sloppy), preferably with a structure that makes it easy to read, identifies/highlights important parts, and otherwise means something semantically (whenever possible).
this section you highlighted includes sql with column names that need to follow the same order as the values provided shortly after; thus the formatting here has line breaks that match up 1:1, so that each row of column names corresponds to a later row for its values. that way, its easy to see (for example) that the last item of the second row of column names (value_updated_timestamp) refers to the last item of the second row of values (123).
all this test code has come a long way, but there is still more room for improvement... see #897
for this pr, i tried not to change anything where i didnt have to, to keep the diff between this and source material as small as possible. the intent was to preserve the spirit of the test refactoring & improvements from the other branch without complicating it by introducing other changes. however, i goofed a couple times and introduced some whitespace changes but ill claim they were for the best ¯\_(ツ)_/¯
|
Is this duplicating some of the work in #927? |
|
@dshemetov half maybe? it will supersede it. target of #927 is v3; target of this PR is v4 |
|
@dshemetov kinda sorta in-a-way but not really, in that #927 duplicated work in #923, then #923 evolved into #931, and this is essentially merging all that from #931 back into the v4 development branch (without the merged key stuff). i did give #927 a good look-through (though not in a very thorough or methodical way) but didnt see anything that seemed too different from what i got from #931. if theres something else there that you think i need to explore, im happy to give it a look! |
Co-authored-by: Katie Mazaitis <[email protected]>
Updates to include improvements from "merged key dimension table" branch (
krivard/v4-rpp-mergeddim-leftjoin), specifically at commit hashfbf878e.Changes include:
This changeset is essentially just a port of the excellent work @krivard did to refactor and otherwise improve the test architecture, as she applied it to branch mentioned above. Most of the files were simply copied over from the other branch to this one to create this PR; I only really made edits to these files: (and most edits were to strip out "mergedkey" stuff)