Skip to content

fix(revenue-analytics): Groups materialized view joins#53119

Merged
arthurdedeus merged 5 commits intomasterfrom
fix/groups-materialized-view-joins
Apr 7, 2026
Merged

fix(revenue-analytics): Groups materialized view joins#53119
arthurdedeus merged 5 commits intomasterfrom
fix/groups-materialized-view-joins

Conversation

@arthurdedeus
Copy link
Copy Markdown
Contributor

@arthurdedeus arthurdedeus commented Apr 1, 2026

Problem

Same as #52929, but for groups.

Part of #52270

Changes

Reuse test setup and util functions to fix the bug for groups.

How did you test this code?

Unit tests

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Publish to changelog?

no

Docs update

no

@arthurdedeus arthurdedeus changed the title test: Split groups managed viewset tests fix(revenue-analytics): Persons materialized view joins Apr 1, 2026
@arthurdedeus arthurdedeus marked this pull request as ready for review April 1, 2026 22:08
@arthurdedeus arthurdedeus requested a review from a team as a code owner April 1, 2026 22:08
@arthurdedeus arthurdedeus changed the title fix(revenue-analytics): Persons materialized view joins fix(revenue-analytics): Groups materialized view joins Apr 1, 2026
@arthurdedeus arthurdedeus self-assigned this Apr 1, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 1, 2026

Prompt To Fix All With AI
This is a comment left during a code review.
Path: posthog/hogql/database/schema/test/test_groups_revenue_analytics.py
Line: 466-469

Comment:
**Redundant `setUp` causes mock leak**

`RevenueAnalyticsManagedViewsetsTestMixin.setUp` already patches `posthoganalytics.feature_enabled` and assigns it to `self.mock_flag`. When `TestGroupsRevenueAnalyticsManagedViewsets.setUp` calls `super().setUp()` first, the mixin's patch is started and `self.mock_flag` is set. Then this method immediately overwrites `self.mock_flag` with a second patch and starts it too.

The result is two active patches for `posthoganalytics.feature_enabled`, but `tearDown` (inherited from the mixin) only calls `self.mock_flag.stop()` once — stopping only the second patch. The first patch leaks, potentially affecting subsequent tests.

The `setUp` override here is already handled by the mixin via `super().setUp()`, so it should be removed entirely. Compare with `TestPersonsRevenueAnalyticsManagedViewsets`, which correctly relies on the mixin's `setUp` without overriding it.

```suggestion
class TestGroupsRevenueAnalyticsManagedViewsets(
    TestGroupsRevenueAnalyticsMixin, RevenueAnalyticsManagedViewsetsTestMixin
):
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix: Support materialized views in group..." | Re-trigger Greptile

Base automatically changed from fix/persons-materialized-view-joins to master April 2, 2026 12:06
@arthurdedeus arthurdedeus force-pushed the fix/groups-materialized-view-joins branch from ae3bf47 to a54fef3 Compare April 2, 2026 12:36
@arthurdedeus arthurdedeus requested a review from mariusandra April 2, 2026 16:49
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

🎭 Playwright report · View test results →

⚠️ 1 flaky test:

  • Materialize view pane (chromium)

These issues are not necessarily caused by your changes.
Annoyed by this comment? Help fix flakies and failures and it'll disappear!

@arthurdedeus arthurdedeus enabled auto-merge (squash) April 7, 2026 17:32
@arthurdedeus arthurdedeus merged commit 80cb5bf into master Apr 7, 2026
281 of 285 checks passed
@arthurdedeus arthurdedeus deleted the fix/groups-materialized-view-joins branch April 7, 2026 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants