Test chart data endpoint w/ duckdb #148
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
| @@ -196,6 +198,7 @@ def _build_query(query_params: dict, filter_groups: list, path_params: dict) -> | |||
| coalesce_columns=columns, | |||
| inline_configs=inline_configs, | |||
| none_configs=none_configs, | |||
| extra_filter_configs=[], # extra_filter_configs | |||
There was a problem hiding this comment.
What's this line about? Did you mean to leave that comment like that?
There was a problem hiding this comment.
That was from an in process attempt to fix the underlying jinja template, which moved away from; will delete
| "version": version.split("__")[2], | ||
| "id": f"{dp_detail['study']}__{dp_detail['name']}__{version.split('__')[2]}", |
There was a problem hiding this comment.
nit: Is version worth splitting separately into a variable, for clarity?
Like _version_package, version_number = version.split("__", 2) or something. (for example, I don't know what the first part of the version is, so I just made up a variable name)
There was a problem hiding this comment.
Yeah, I buy it
| return value | ||
|
|
||
| db = duckdb.connect(tmp_path / "duck.db") | ||
| db.create_function( |
There was a problem hiding this comment.
Obviously you thought about this, but let's spill a tiny amount of digital ink. Pros/cons of having tests depend on cumulus-library and grabbing its DuckdbDatabase class? Not worth it yet, I'm guessing?
There was a problem hiding this comment.
Yeah, a couple thoughts
- the aggregator doesn't have Python version matrix compatibility problems. It runs on a specific version and there's some workaround code for Python 311 in there that just doesn't apply here.
- there's one weird case where we want this to perform slightly differently around, just not caring about finding real time stamps in the database with test data
- we're only dealing with this one function for now, so the library class is a little heavy and having its own thing provides a nice isolation. If that changed that I might feel more strongly about importing that class.
This adds a duckdb fixture to the aggregator and uses it for testing the chart data endpoint, and fixes several issues found as a result.