Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
phyllis-sy-wu
force-pushed
the
psyw-0331-E2E-infra-refactor
branch
3 times, most recently
from
56f9281 to
756b888
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the packages/e2e Playwright fixture infrastructure and migrates app-related E2E tests to be self-contained by creating their own apps (via --organization-id) and performing best-effort cleanup via the Partner dashboard, reducing reliance on pre-provisioned apps and improving CI robustness when SHOPIFY_CLI_PARTNERS_TOKEN is present.
Changes:
- Introduces a new fixture chain with a worker-scoped persistent browser page (
browserFixture) and updates auth to run on that shared page. - Replaces the old app scaffold fixture/class with composable CLI + browser helper functions (create/build/deploy + dashboard cleanup helpers).
- Migrates app scaffold/dev/deploy tests to create fresh apps using
E2E_ORG_ID, and adds.envloading helper + CI env updates for E2E.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/e2e/tests/app-scaffold.spec.ts | Migrates scaffold tests to createApp/buildApp helpers and adds best-effort cleanup. |
| packages/e2e/tests/app-dev-server.spec.ts | Migrates dev-server test to create a fresh app + cleanup in finally. |
| packages/e2e/tests/app-deploy.spec.ts | Migrates deploy test to create/deploy/list versions using new helpers + cleanup. |
| packages/e2e/setup/env.ts | Adds orgId to E2EEnv and wires E2E_ORG_ID through requireEnv and fixtures. |
| packages/e2e/setup/browser.ts | Adds worker-scoped browserPage fixture (persistent Chromium page). |
| packages/e2e/setup/auth.ts | Updates auth fixture to reuse browserPage and strips partners token (needs a small fix). |
| packages/e2e/setup/app.ts | Replaces AppScaffold with composable CLI/browser helpers + appTestFixture. |
| packages/e2e/playwright.config.ts | Uses shared loadEnv helper instead of inline parsing. |
| packages/e2e/helpers/load-env.ts | New .env loader supporting quotes and inline comments. |
| .github/workflows/tests-pr.yml | Adds E2E_ORG_ID and SHOPIFY_CLI_PARTNERS_TOKEN to E2E test step env. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Env override applied to all CLI helpers — strips CLIENT_ID so commands use the app's own toml. | ||
| // NOTE: Do NOT add SHOPIFY_CLI_PARTNERS_TOKEN here. The partners token overrides OAuth in the | ||
| // CLI's auth priority, and its App Management API exchange can't create apps. OAuth handles everything. | ||
| const FRESH_APP_ENV = {SHOPIFY_FLAG_CLIENT_ID: undefined} |
There was a problem hiding this comment.
this is a hack. just remove the env variable if we're no longer using it
There was a problem hiding this comment.
I wanted to keep this PR's scope focused on the infra + app tests so it doesn't get too complicated.
So this PR only migrates the app-relevant tests (app-deploy, app-dev-server, app-scaffold) from using pre-existing apps to freshly created ones. The untouched tests still rely on SHOPIFY_FLAG_CLIENT_ID to reference their pre-existing apps. FRESH_APP_ENV scopes the SHOPIFY_FLAG_CLIENT_ID stripping to just the app helpers so we don't break those tests. My plan is to migrate all remaining tests in follow-up PRs, at which point both FRESH_APP_ENV and SHOPIFY_FLAG_CLIENT_ID can be removed entirely.
Does this makes sense?
|
|
||
| const result = await cli.execCreateApp(args, { | ||
| // Strip CLIENT_ID so the CLI creates a new app instead of linking to a pre-existing one | ||
| env: {FORCE_COLOR: '0', ...FRESH_APP_ENV}, |
There was a problem hiding this comment.
we should never be messing with the loaded .env or short circuiting it to pass a different .env
There was a problem hiding this comment.
Same reasoning as here. Need this override because the untouched tests still use SHOPIFY_FLAG_CLIENT_ID. Once those are migrated in a follow-up PR, this override goes away too.
phyllis-sy-wu
force-pushed
the
psyw-0331-E2E-infra-refactor
branch
2 times, most recently
from
603fe61 to
019cbfd
Compare
phyllis-sy-wu
force-pushed
the
psyw-0331-E2E-infra-refactor
branch
from
b931123 to
bf1e8b6
Compare
phyllis-sy-wu
force-pushed
the
psyw-0331-E2E-infra-refactor
branch
from
bf1e8b6 to
de26577
Compare
WHY are these changes introduced?
The main goal of this PR is to strengthen the E2E infrastructure by addressing four areas:
Reusable helpers — Added composable CLI helpers (
createApp,buildApp,deployApp, etc.) and browser helpers (navigateToDashboard,uninstallApp,deleteApp, etc.) that future tests can build on.Self-contained app tests — The existing E2E app tests depend on pre-provisioned apps via
SHOPIFY_FLAG_CLIENT_ID. This makes tests fragile — they break if the pre-provisioned app is deleted, modified, or its org permissions change. This PR migrates app-relevant tests to create their own apps during the test run using--organization-id, removing the dependency on fixedSHOPIFY_FLAG_CLIENT_IDvalues.Automatic cleanup — Tests that create apps pollute the organization if not cleaned up. Each test now cleans up after itself in a
finallyblock — deleting local files and removing the app from the partner dashboard (uninstall from stores + delete). This is best-effort (with DEBUG logging) so cleanup failures never fail the test.Auth robustness — The
SHOPIFY_CLI_PARTNERS_TOKENis not passed to the E2E test step in CI. When present, the CLI prioritizes it over OAuth and exchanges it for an App Management API token that lacks permission to create apps (403). By keeping it out of the E2E environment, all tests use the OAuth session consistently.WHAT is this pull request doing?
New fixture chain:
envFixture → cliFixture → browserFixture → authFixture → appTestFixturesetup/browser.ts(new) — Worker-scoped Chromium browser page, persistent across tests for OAuth login and dashboard cleanup. ExportsBrowserContexttype andnavigateToDashboardhelper (generic dashboard navigation with account picker handling and 500 retry).setup/auth.ts(modified) — Now extendsbrowserFixture. Performs OAuth login using the shared browser page.setup/cli.ts(modified) — ExportsCLIContexttype (used by app helpers).setup/app.ts(rewritten) — Replaces the oldAppScaffoldclass with composable helper functions:createApp,generateExtension,buildApp,deployApp,appInfo,functionBuild,functionRun,versionsList,configLink— all take a single object arg, all useFRESH_APP_ENVto stripCLIENT_IDfindAppsOnDashboard,uninstallApp,deleteApp,teardownApp— for best-effort per-test app cleanup via the Dev Dashboard (with DEBUG logging on failure)setup/env.ts(modified) — AddedorgIdtoE2EEnvinterface, fixture, andrequireEnvplaywright.config.ts(modified) — Usesdotenvpackage for.envloadingpackage.json— AddeddotenvdevDependencyMigrated tests:
app-scaffold.spec.ts— creates fresh apps withorgId, cleans up infinallyapp-deploy.spec.ts— creates app, deploys, verifies version, cleans upapp-dev-server.spec.ts— creates app, starts dev server, verifies ready message, quits withq, cleans upUntouched tests (dev-hot-reload, multi-config-dev, toml-config) continue to work via the OAuth session stored on disk by the app tests.
CI workflow:
.github/workflows/tests-pr.yml— AddedE2E_ORG_IDto the E2E test step environmentHow to test your changes?
Measuring impact
Checklist