Add abstract test classes #2754
Conversation
posthog/queries/test/test_trends.py
Outdated
| @@ -166,6 +168,210 @@ def test_trends_compare(self): | |||
| self.assertEqual(no_compare_response[0]["labels"][5], "Thu. 2 January") | |||
| self.assertEqual(no_compare_response[0]["data"][5], 1.0) | |||
|
|
|||
| def _test_interval_filtering(self, dates: List[str], result, **filter_params): | |||
There was a problem hiding this comment.
stricly implementing these tests is already showing discrepancies between postgres and clickhouse query results that weren't considered before.
| interval="minute", | ||
| date_from="2020-11-01 10:20:00", | ||
| date_to="2020-11-01 10:30:00", | ||
| result=[ |
There was a problem hiding this comment.
testing the entire object should be preferred because it will keep the result between clickhouse and postgres very consistent
|
My original thought was something like class TestRetention:
def test_breakdown_cohort_property(self):
data = super().test_breakdown_cohort_property()
filter = RetentionFilter(data=data)
query = Retention.run(filter=filter)
self.assertEqual(query, [...])Or even class TestRetention(AbstractBreakdownTest):
filter = RetentionFilter
query = Retention
test_breakdown_cohort_property = [...]
test_breakdown_multiple_properties = [...]where the mixing takes care of setting up the events etc. However this is a great first step just ensuring all queries test everything |
|
Do you think the event data should be produced in such a way that is reusable across different tests? I came around to think that the data should be generated per test like we're doing now because it's easier to diagnose issues. If we have a giant data initializer, when a test is broken, it's going to be difficult to look through the data to see what data is related to the test. |
|
Hm yeah that's fair, though I sometimes think we miss things because of that (like having one test attributed to another team would have caught a couple of team leakage issues). Also in reality you'll always have the same set of data to run queries over, rather than data tailored to each query. It also avoids a ton of boilerplate code. Having said that it's def not a blocker for this PR. |
Changes
Please describe.
If this affects the frontend, include screenshots.
Checklist