feat(experiments): Restore internal user filter for dw #26911
Conversation
|
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
That's great! |
| _create_event( | ||
| team=self.team, | ||
| event="$feature_flag_called", | ||
| distinct_id="internal_test_1", | ||
| properties={ | ||
| feature_flag_property: "test", | ||
| "$feature_flag_response": "test", | ||
| "$feature_flag": feature_flag.key, | ||
| "$user_id": "internal_test_1", | ||
| }, | ||
| timestamp=datetime(2023, 1, 3), | ||
| ) |
There was a problem hiding this comment.
I’d prefer a clearer test to explicitly verify that this event is excluded from the returned data. It might be better to have a separate method specifically for testing the filtering of test users.
There was a problem hiding this comment.
How might you suggest constructing the test?
There was a problem hiding this comment.
Something like:
- Only populate the data warehouse records with rows including the test user id
- The experiment query should return no result
My point is: we’re creating a test user and an event generated by them. Am I missing something, or what exactly are we doing with this user and event later on? How are we ensuring that this event is excluded from the results?
There was a problem hiding this comment.
How are we ensuring that this event is excluded from the results?
It's inferred. These values will change significantly if the event is included in results:
The way I went about writing the test was:
- Add the data warehouse row and event so the test was failing.
- Add the persons table join and
filterTestAccounts=Truestatement so the test is passing with the original conditions again.
I think what you suggest is marginally more explicit, but could also be unexpectedly erroneous in some other ways.
I updated the test to also run the query with filterTestAccounts=false and assert the results: 5ca3fd3
| @@ -906,7 +946,7 @@ def test_query_runner_with_data_warehouse_series_no_end_date_and_nested_id(self) | |||
| ) | |||
|
|
|||
| # Assert the expected join condition in the clickhouse SQL | |||
| expected_join_condition = f"and(equals(events.team_id, {query_runner.count_query_runner.team.id}), equals(event, %(hogql_val_8)s), greaterOrEquals(timestamp, assumeNotNull(parseDateTime64BestEffortOrNull(%(hogql_val_9)s, 6, %(hogql_val_10)s))), lessOrEquals(timestamp, assumeNotNull(parseDateTime64BestEffortOrNull(%(hogql_val_11)s, 6, %(hogql_val_12)s))))) AS e__events ON" | |||
| expected_join_condition = f"and(equals(events.team_id, {query_runner.count_query_runner.team.id}), equals(event, %(hogql_val_12)s), greaterOrEquals(timestamp, assumeNotNull(parseDateTime64BestEffortOrNull(%(hogql_val_13)s, 6, %(hogql_val_14)s))), lessOrEquals(timestamp, assumeNotNull(parseDateTime64BestEffortOrNull(%(hogql_val_15)s, 6, %(hogql_val_16)s))))) AS e__events ON" | |||
There was a problem hiding this comment.
Just thinking out loud - how fragile is it to have a test set up like this, where we rely on checking the generated Clickhouse SQL? This could break if the hogql_val names change, even though it wouldn't actually affect our functionality.
There was a problem hiding this comment.
Correct: it does break when the hogql_val names change. I'm fine with that right now, though. It's a brittle test for a brittle piece of code, and I'd like for it to break noisily if anything changes.
I added a more descriptive exception message for anyone who runs into it though: 9fb4210
See #26332
See https://posthog.slack.com/archives/C06KJQ2RH1A/p1734117119825209?thread_ts=1733936000.594629&cid=C06KJQ2RH1A
Changes
Reverts #26818 and adds some tests
Restores the ability to use the test and internal user filter for data warehouse experiments. As it turns out, it works fine if you set up a join to the persons table.
How did you test this code?
Tests should pass.