QA to E2E: app basic flow (no extensions)#7047
Conversation
Coverage report
Test suite run success3910 tests passing in 1502 suites. Report generated by 🧪jest coverage report action from f3e68db |
phyllis-sy-wu
force-pushed
the
psyw-0318-QAtoE2E-basic-flow
branch
from
09c1991 to
04d93f8
Compare
phyllis-sy-wu
force-pushed
the
psyw-0318-QAtoE2E-basic-flow
branch
from
04d93f8 to
11a9ece
Compare
phyllis-sy-wu
force-pushed
the
psyw-0318-QAtoE2E-basic-flow
branch
from
b215880 to
f3e68db
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new Playwright end-to-end test in packages/e2e to automate the “basic app lifecycle” QA flow (init → dev → execute → quit → dev clean → deploy → versions → config link → deploy via secondary config), plus supporting fixture and dependency updates to make the flow reliable in CI.
Changes:
- Introduces
app-basic.spec.tsto cover the full app workflow including secondary config link + deploy. - Updates the e2e CLI fixture to avoid inheriting the parent shell environment (
extendEnv: false). - Adds a
@shopify/cli-kitdevDependency topackages/e2eto supportjoinPathusage.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Lockfile updates for the new @shopify/cli-kit devDependency and resulting dependency graph changes. |
| packages/e2e/tests/app-basic.spec.ts | New end-to-end “basic flow” test including config link + secondary deploy. |
| packages/e2e/setup/cli.ts | Adjusts execa options to prevent env leakage by disabling env extension. |
| packages/e2e/setup/auth.ts | Adds a console log indicating automatic authentication. |
| packages/e2e/setup/app.ts | Avoids parsing scaffold output when create-app exits non-zero (returns early). |
| packages/e2e/package.json | Adds @shopify/cli-kit devDependency needed by the new test. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const execaOpts: ExecaOptions = { | ||
| cwd: opts.cwd, | ||
| env: {...env.processEnv, ...opts.env}, | ||
| extendEnv: false, | ||
| timeout, | ||
| reject: false, | ||
| } |
There was a problem hiding this comment.
CLIProcess.exec/execCreateApp pass {...env.processEnv, ...opts.env} directly to execa. If a test sets an env key to undefined (used in this PR to “unset” SHOPIFY_FLAG_CLIENT_ID), Node will coerce it to the string 'undefined', so the variable remains effectively set and can break flag/env resolution. Filter out undefined values (similar to spawn()), or explicitly delete keys with undefined before calling execa.
| import {appScaffoldFixture as test} from '../setup/app.js' | ||
| import {requireEnv} from '../setup/env.js' | ||
| import {expect} from '@playwright/test' | ||
| import {joinPath} from '@shopify/cli-kit/node/path' | ||
| import * as fs from 'fs' | ||
|
|
There was a problem hiding this comment.
This test imports Node built-ins (fs) but doesn’t include the /* eslint-disable no-restricted-imports */ header that other e2e tests/fixtures use when importing fs/path. Since packages/e2e’s lint target includes tests/**/*.ts, this is likely to fail lint; add the disable (or switch to the preferred abstractions per the restricted-import rule).
| '--message', | ||
| 'E2E secondary app deployment', | ||
| ], | ||
| {timeout: 5 * 60 * 1000, env: {SHOPIFY_FLAG_CLIENT_ID: undefined}}, |
There was a problem hiding this comment.
Passing env: {SHOPIFY_FLAG_CLIENT_ID: undefined} to cli.exec(...) won’t actually unset the variable with the current CLIProcess.exec implementation; it will be forwarded to the child process and can become the string 'undefined'. Prefer omitting the key entirely, or update CLIProcess.exec to drop undefined env entries (as spawn() already does).
| {timeout: 5 * 60 * 1000, env: {SHOPIFY_FLAG_CLIENT_ID: undefined}}, | |
| {timeout: 5 * 60 * 1000}, |
| if (!email || !password) { | ||
| await use() | ||
| return | ||
| } | ||
|
|
||
| process.stdout.write('[e2e] Authenticating automatically — no action required.\n') | ||
|
|
There was a problem hiding this comment.
PR description says the "Authenticating automatically" log should appear for both OAuth and partners-token auth paths, but this message is only printed when E2E_ACCOUNT_EMAIL/E2E_ACCOUNT_PASSWORD are set. If the partners-token path is intended to log too, move/add the log before the early return or condition it on env.partnersToken.
| }, | ||
| "devDependencies": { | ||
| "@playwright/test": "^1.50.0", | ||
| "@shopify/cli-kit": "3.92.0", |
There was a problem hiding this comment.
I don't think we want to include cli-kit in e2e. e2e should test the public exposed behavior via the cli binary itself, it shouldn't need to be aware of any cli internals.
| import {appScaffoldFixture as test} from '../setup/app.js' | ||
| import {requireEnv} from '../setup/env.js' | ||
| import {expect} from '@playwright/test' | ||
| import {joinPath} from '@shopify/cli-kit/node/path' |
There was a problem hiding this comment.
you can just use node directly in e2e
| {timeout: 60 * 1000}, | ||
| ) | ||
| const executeOutput = executeResult.stdout + executeResult.stderr | ||
| expect(executeResult.exitCode, '‼️ Step 3 - app execute failed').toBe(0) |
There was a problem hiding this comment.
a little unclear -- we expect exit code to be 0, but the text is asserting the operation failed?
WHY are these changes introduced?
To replace manual CLI QA with automated end-to-end tests that validate the complete app development workflow in a real environment.
This is part of Vault project #gsd:49408 to automate CLI QA coverage.
WHAT is this pull request doing?
Adds
tests/app-basic.spec.ts: a full lifecycle e2e test covering 9 steps:app initapp devCI='')app executeqkeyqkeystroke to terminate the dev serverapp dev cleanapp deployapp versions list --jsonapp config link --client-id <secondary>shopify.app.secondary.tomlstub (client_id = "...") before spawning, sogetTomls()finds the secondary client ID andloadConfigurationFileNamereturns the filename immediately without triggering the interactive "Configuration file name:" promptapp deploy --config secondarySHOPIFY_FLAG_CLIENT_IDunset to avoid the--config/--client-idmutual exclusion enforced by oclifSupporting changes:
setup/cli.ts: addsextendEnv: falsetoexecandexecCreateAppso the test's explicitprocessEnvis the complete environment — no leakage from the parent shellsetup/auth.ts: logs "Authenticating automatically" for both the browser OAuth path and the partners-token path (was missing from the token path)package.json: adds@shopify/cli-kit: 3.92.0as a devDependency (required for thejoinPathimport, pinned to match all other packages per the repo-health version sync check)Divergence from the manual flow
In the manual flow:
app initcreates a new app (you pick org, name a new app)app devlinks to that new appapp deploydeploys to that new appIn the e2e test:
app initscaffolds the local directory but links to a pre-existing app viaSHOPIFY_FLAG_CLIENT_IDWhy the test does it this way:
client_idskips all that interactivityThe tradeoff:
app initHow to test your changes?
Running the test
Requires
.envwithSHOPIFY_FLAG_CLIENT_ID,E2E_STORE_FQDN,E2E_SECONDARY_CLIENT_IDVerifying each step catches failures
pnpm nx build app --skip-nx-cache && nx build cli --skip-nx-cache‼️ Step N - <command> failedpnpm nx build app --skip-nx-cache && nx build cli --skip-nx-cacheapp initapp devapp executeqkeyapp dev cleanshopify app deploy --version v1app versions list --jsonapp config link --client-id <secondary>app deploy --config secondaryMeasuring impact
Checklist