search

dbt-labs / metricflow

MetricFlow allows you to define, build, and maintain metrics in code.
https://docs.getdbt.com/docs/build/about-metricflow
Other
1.12k stars 92 forks source link

Tests for `Metric.time_granularity` #1325

Closed courtneyholcomb closed 1 month ago

courtneyholcomb commented 1 month ago

Adds default_granularity tests for query parsing, SQL rendering, query output, and check query tests. Also cleans up a couple of comments due to product decisions that changed the expected implementation of this feature.

I will add more tests once we finalize the expected behavior for more specific scenarios.

plypaul commented 1 month ago

Following what you noted in the description - are you planning to add more items to this PR or has that already been done?

courtneyholcomb commented 1 month ago

Following what you noted in the description - are you planning to add more items to this PR or has that already been done?

@plypaul planning to add them in a separate PR!

courtneyholcomb commented 1 month ago

@plypaul

  • Is there a specific reason why the default granularity was updated for those metrics? It has resulted in a number of snapshot changes, so I am wondering if there's an interaction between those metrics and default granularity that's important to check.

I chose to add time_granularity to those metrics to that we could see the behavior for a few different types of metrics that I thought might behave differently, like a simple metric vs. a metric with input metrics (the ratio metric). The snapshot changes were intentional. I thought it would be helpful to include this feature in some tests that were not just unit tests. I went through the snapshot changes before putting up the PR before putting up the PR and thought they made sense based on the query inputs. What snapshot changes are you seeing that concern you?

Sure, I can add some tests there!

courtneyholcomb commented 1 month ago

@plypaul moved those query parser tests, and separated out the version bump into a new PR.

plypaul commented 1 month ago

What snapshot changes are you seeing that concern you?

It was more that I was wondering whether there as any particular interaction between the default granularity and the fill configuration option.

courtneyholcomb commented 1 month ago

What snapshot changes are you seeing that concern you?

It was more that I was wondering whether there as any particular interaction between the default granularity and the fill configuration option.

Nope, there shouldn't be. Just happened to be that the metrics I added a time granularity to were used in those tests.