chore(devex): revert opt-in playwright e2e tests#60154
Merged
Merged
Conversation
Reverts the label-gated opt-in behavior from #57657. PRs that touch E2E-affecting paths now always run Playwright, matching pre-#57657 behavior. Keeps the schema-cache restore step, runner-size change, and the `db:restore-schema-fresh` hogli rename from the same PR — those are independent of the opt-in mechanism. Drops: - `labeled` trigger and the `run-playwright` label gating - `mustRun` / `shouldSuggest` split (back to a single `shouldRun` filter) - `suggest-label` job that nudged authors to add the label Generated-By: PostHog Code Task-Id: 90595834-ca5a-415a-8b7d-631253a62c18
Member
Author
|
🤖 CI status note from the agent that opened this PR: The only failing checks are This is the Per the Slack thread that prompted this PR, this is exactly the breakage that opt-in was hiding. Surfacing it is the point of the revert. The fix needs to land in the credential-review feature or its tests — both out of scope for this PR, which only touches the workflow file. I'm not making those changes here. Leaving the PR as draft so humans can decide whether to land this before or after the credential-review fix. |
- Detach conflicting Kafka-engine tables from the dev `posthog` ClickHouse DB during reset-db so `posthog_test.kafka_person` (and friends) can claim the `clickhouse_person` partition instead of losing the consumer-group race in `group1` — fixes "Person data did not land in ClickHouse" hang. - Mark playwright workspace users as `credentials_reviewed_at=now()` after minting their PersonalAPIKey so the post-login `/account/credential-review` interstitial no longer hijacks the test session. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Size Change: 0 B Total Size: 80.2 MB ℹ️ View Unchanged
|
UserSerializer.get_requires_credential_review now short-circuits to False when settings.E2E_TESTING is set. E2E test users always carry a seeded PersonalAPIKey, so the post-login interstitial would otherwise hijack every test session and block scenes from rendering. Defense-in-depth on top of the setup_dev / playwright_setup_functions paths that already mark new users reviewed — catches users created before those code paths landed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`.scene-tab-row .scene-tab-title` is no longer rendered — the only remaining reference is a `querySelector` lookup in sceneLogic.tsx that returns nothing. Target the H1 rendered by SettingsScene.tsx instead, which still emits the exact "Settings - Profile" / "Settings - General" strings the test asserts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Reverts the opt-in/label-gated trigger mechanism for the Playwright E2E workflow introduced in #57657, restoring automatic runs on PRs that touch E2E-affecting paths. Keeps the schema-cache restore step, the 8-core runner downsize, and the db:restore-schema-fresh rename. The PR also bundles several seemingly-unrelated fixes to make the E2E suite green (credential-review interstitial bypass, a Playwright selector update, and a ClickHouse Kafka-table workaround in bin/mprocs-e2e.yaml) that are not called out in the description.
Changes:
- Drops the
labeledPR trigger andrun-playwrightlabel gating; collapsesmustRun/shouldSuggestinto a singleshouldRunpaths filter; removes thesuggest-labeljob. - Adds an
E2E_TESTINGshort-circuit inUserSerializer.get_requires_credential_reviewand seedscredentials_reviewed_aton test users to skip the credential-review interstitial. - Updates a Playwright assertion to a role-based selector and detaches Kafka-engine tables from the
posthogClickHouse DB before E2E migrations.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/ci-e2e-playwright.yml | Reverts opt-in label trigger, label-gated if, dual paths filter, and suggest-label job. |
| posthog/api/user.py | Adds an unconditional E2E_TESTING bypass returning False from get_requires_credential_review. |
| posthog/test/playwright_setup_functions.py | Seeds credentials_reviewed_at = now() on created test users to skip the credential-review interstitial. |
| playwright/e2e/before-onboarding.spec.ts | Switches the settings-page assertion from a CSS scene-tab selector to getByRole('heading', { level: 1 }). |
| bin/mprocs-e2e.yaml | Before migrations, detaches all Kafka-engine tables in the posthog ClickHouse DB to avoid consumer-group group1 conflicts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
posthog/api/user.py:384-390
The `settings.E2E_TESTING` short-circuit is now redundant: `playwright_setup_functions.py` already sets `credentials_reviewed_at` on every test user immediately after creating the API key, so the `if instance.credentials_reviewed_at is not None: return False` branch handles the same case. Keeping both expresses the same fix twice and leaves the setup-function change invisible from the serialiser's perspective — a reader maintaining this code later may wonder which guard is the authoritative one.
```suggestion
@tracer.start_as_current_span("user_serializer.requires_credential_review")
def get_requires_credential_review(self, instance: User) -> bool:
if instance.credentials_reviewed_at is not None:
return False
return PersonalAPIKey.objects.filter(user=instance).exists()
```
Reviews (1): Last reviewed commit: "test(before-onboarding): replace removed..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
⏭️ Skipped snapshot commit because branch advanced to The new commit will trigger its own snapshot update workflow. If you expected this workflow to succeed: This can happen due to concurrent commits. To get a fresh workflow run, either:
|
Sets PERSON_ON_EVENTS_OVERRIDE=1 on the mprocs-e2e backend env so test teams resolve to PERSON_ID_NO_OVERRIDE_PROPERTIES_ON_EVENTS instead of the slow PERSON_ID_OVERRIDE_PROPERTIES_JOINED fallback. Without this, self-hosted/dev teams default to the JOIN-based mode (team.py:751) and trends queries take longer than the 10s LemonTableLoader wait, leaving the details table stuck on a skeleton loader. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
playwright/e2e/scene-tabs.spec.ts:1
- Several changes in this diff are not mentioned in the PR description, which advertises only the workflow revert and three specific additional fixes:
- Deletion of
playwright/e2e/scene-tabs.spec.ts(a whole test file). - Selector changes in
playwright/e2e/surveys/quickcreate.spec.ts(.scene-tab-title→[data-attr="scene-name"]). - Adding
PERSON_ON_EVENTS_OVERRIDE: '1'tobin/mprocs-e2e.yaml. - Wrapping
infer_taxonomy_for_teamintags_context(...)inposthog/test/playwright_setup_functions.py.
Each of these has user-facing or behavioral implications (especially deleting a test and changing a query-tagging context) that reviewers won't be expecting. Either trim them out of this PR or expand the description to cover them and explain the rationale.
DanielVisca
approved these changes
May 28, 2026
Contributor
DanielVisca
left a comment
DanielVisca
left a comment
There was a problem hiding this comment.
Yay to no more broken tests!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Slack discussion (thread) raised that making Playwright E2E tests opt-in via the
run-playwrightlabel (#57657) has not worked in practice — when E2E broke, opt-in meant the breakage went unnoticed, and there were 3 separate reasons E2E was failing before #60101 went in. Robbie noted in the thread that opt-in has failed as a strategy.This PR reverts the opt-in mechanism so Playwright again runs automatically on PRs that touch E2E-affecting paths.
While verifying the revert locally I hit a cascade of pre-existing E2E breakages that prevented
playwrightSetup.createWorkspace-based tests from rendering anything at all. Those fixes are bundled here so the workflow revert isn't ineffective on day one — full detail in the "Additional fixes" section below.Changes
Workflow revert (the headline change)
Reverts the opt-in pieces introduced by #57657 in
.github/workflows/ci-e2e-playwright.yml:labeledPR trigger and therun-playwrightlabel gating in thechangesjob.mustRun/shouldSuggestback into a singleshouldRunpaths filter (union of the two previous lists). Any change matching the filter triggers a run.suggest-labeljob that nudged authors to add the label.Keeps unrelated, useful changes from the same PR:
depot-ubuntu-latest-8(separate empirical decision per revert(ci): remove Playwright E2E sharding - setup cost too high #46853).db:restore-schema-freshhogli rename and its callers inci-backend.yml/ci-dagster.yml.Additional fixes (unblock E2E locally + in CI once it runs again)
posthog/test/playwright_setup_functions.py— setsuser.credentials_reviewed_at = timezone.now()immediately after minting the seeded Test API Key increate_organization_with_team. Defense-in-depth alongside the serializer gate — works even outsideE2E_TESTINGmode (e.g. devbox runs against a non-E2E_TESTINGbackend).bin/mprocs-e2e.yaml— thereset-dbproc now detaches everyKafka-engine table from theposthogClickHouse DB (DETACH TABLE ... PERMANENTLY) after recreatingposthog_test. Two CH databases withkafka_personconsumers in the same Kafka consumer groupgroup1race for the single-partitionclickhouse_persontopic — the devposthogDB consumer was winning, so e2e demo persons never landed inposthog_test.personand the setup endpoint hung 120s before returning 500 ("Person data did not land in ClickHouse"). Non-destructive: dev data is preserved, only the consumers are detached.playwright/e2e/before-onboarding.spec.ts— the.scene-tab-row .scene-tab-titleselector no longer matches anything (only stalequerySelectorreference insceneLogic.tsx). Switched togetByRole('heading', { level: 1 }), which is whatSettingsScene.tsxactually renders for "Settings - Profile" / "Settings - General".How did you test this code?
I'm an agent. I did not run the workflow itself locally.
Workflow revert — verified statically:
run-playwright/shouldSuggest/suggest-label/mustRunidentifiers.shouldRunoutput is consumed only by theplaywrightjob'sif:andcapture-run-time'sneeds:; both remain wired correctly.schema_cache_keyoutput and downstream cache-restore step are preserved.Additional fixes — verified against the local
mprocs-e2estack:setup_test/organization_with_team/endpoint now returns HTTP 200 in ~3.5s (was HTTP 500 after 124.8s).system.kafka_consumersshowsposthog_test.kafka_person.assignments.topicpopulated where it was previously empty.test@posthog.comseeded user is no longer bounced to/account/credential-reviewafter theE2E_TESTINGgate landed.getByRole('heading', { level: 1 })matches the<h1>rendered bySettingsScene.tsx:35, which still emits the exact strings the test asserts.Publish to changelog?
No — CI / test-infra only change.
🤖 Agent context
Authored by Claude (Opus 4.7) at Georges-Antoine's request after the Slack thread above. Considered a full revert of #57657 but rejected it because that PR also rewrote
db:restore-test-db→db:restore-schema-freshand updatedci-backend.yml/ci-dagster.ymlto call the new name; a full revert would break those workflows. Targeted only the opt-in mechanism instead.The additional fixes accumulated while trying to confirm the revert actually helped — each one unblocked a different layer of the E2E stack (workspace setup, credential-review redirect, broken test selector). Bundled into this PR so that re-enabling auto-run doesn't immediately produce a red CI on master.
Created with PostHog Code