Refactor: Replace network-dependent test fixture with local testdata in convert_metrics #1307
Closed
Syedowais312
started this conversation in
Ideas
Replies: 2 comments 1 reply
-
|
hey! thanks for your suggestion! I will remove the utility completely. Enough time passed since v2, so there is no need in keeping it. |
Beta Was this translation helpful? Give feedback.
1 reply
-
|
Closing as completed! Thank you. |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Hey, I've been looking at the integration test in
cmd/convert_metrics/convert_metrics_test.goand noticed it currently downloads a zip from GitHub at test runtime to get the metrics fixtures. This makes the test slow and flaky (network dependency, GitHub rate limits, zip structure changes could silently break it).I refactored it to use a local
testdata/metrics/fixture instead a minimal but representative set covering:example_metric/11/metric_master.sql)init_sql(00_helpers/helper_fn/11/metric.sql)basic) that references the metricThe test runs in ~0.005s now vs waiting on a network round trip, and the assertions verify the core converter behavior (metrics, init_sql, presets) still works correctly.
Should I go ahead and open a PR for this? Happy to expand the testdata if the current fixture feels too minimal, or keep it as-is if the goal is just to verify the converter wiring rather than exhaustive coverage.
Was the pgwatch2 URL intentional i.e. is the converter meant to handle real pgwatch2 metrics as input?
Let me know your thoughts.
CC: @pashagolub @0xgouda
Beta Was this translation helpful? Give feedback.
All reactions