Conversation
|
Preview deployments are available for this PR. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a Playwright-based E2E suite to validate core user flows (Auth + bucket/object operations) against a deployed SST stage, and wires CI to run it reliably by pre-authing roles and reseeding billing state in DynamoDB.
Changes:
- Added Playwright E2E specs for login/logout, dashboard visibility by role, and bucket/object flows (with role-specific expectations).
- Introduced an
auth.setup.tsproject to log in each role once and persist storageState, plus a DynamoDB-based billing reseed helper. - Updated CI workflows, docs, and scripts to run E2E inside
sst shellwith required env vars and OIDC AWS credentials.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/roles.ts | Defines role → storageState file mapping used across E2E. |
| tests/e2e/auth.setup.ts | Playwright setup project: reseeds billing + logs in each role and writes .auth/*.json. |
| tests/e2e/billing-reset.ts | Writes desired billing subscription state directly to BillingTable before tests. |
| tests/e2e/login.spec.ts | UI login smoke test (no storageState) to detect auth setup regressions. |
| tests/e2e/logout.spec.ts | Logout flow test + cookie clearing assertions. |
| tests/e2e/dashboard.spec.ts | Role-based dashboard assertions. |
| tests/e2e/buckets.spec.ts | Bucket/object upload flows (create tests currently skipped) by role. |
| tests/e2e/example.spec.ts | Removes the default example spec. |
| playwright.config.ts | Adds required-credentials validation and a setup project dependency chain. |
| package.json | Runs Playwright via sst shell to resolve SST Resource bindings. |
| README.md | Documents required env vars, local/CI setup, billing reset, and role additions. |
| .gitignore | Ignores generated .auth storageState directory. |
| .github/workflows/e2e-staging.yaml | Adds staging env vars + OIDC AWS creds to run E2E and reseed BillingTable. |
| .github/workflows/test-staging.yaml | Same as above for the staging test workflow job. |
| pnpm-lock.yaml | Locks new @aws-sdk/client-dynamodb dev dependency. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await page.getByRole('button', { name: 'User menu' }).click(); | ||
| await page.getByRole('button', { name: 'Log out' }).click(); | ||
|
|
||
| await expect(page).toHaveURL(/^https:\/\/fil\.one\/?$/); |
There was a problem hiding this comment.
toHaveURL(/^https:\/\/fil\.one\/?$/) hard-codes the production domain, but the suite is documented to run against arbitrary BASE_URL (e.g. staging). This will fail when running against staging/preview. Assert against the configured baseURL (or just toHaveURL(/\/$/) after page.goto('/')), and keep the domain flexible.
| await expect(page).toHaveURL(/^https:\/\/fil\.one\/?$/); | |
| await expect(page).toHaveURL(/\/$/); |
There was a problem hiding this comment.
We're always redirected to this domain.
There was a problem hiding this comment.
Yeah, to add to your answer, this is due to the fact we have no staging version of marketing site.
| async function openFirstBucket(page: Page): Promise<string> { | ||
| await page.goto('/buckets'); | ||
| const firstBucketLink = page.locator('tbody a[href^="/buckets/"]').first(); | ||
| await expect(firstBucketLink).toBeVisible(); | ||
| await firstBucketLink.click(); | ||
| await page.waitForURL(/\/buckets\/[^/]+$/); | ||
| return new URL(page.url()).pathname.split('/').pop()!; | ||
| } |
There was a problem hiding this comment.
openFirstBucket() assumes the account already has at least one bucket. Because the bucket-creation tests are currently skipped (and buckets aren’t deletable), this makes the upload tests fail on fresh test users/environments with zero buckets and can become flaky over time as buckets are exhausted. Consider making the target bucket explicit via an env var, or in setup ensure a reusable bucket exists (create once if absent, otherwise reuse), and skip with a clear message if none exist.
There was a problem hiding this comment.
Since we're always using the same accounts we should be good.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…yperspace into feat/add-e2e-suite
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 15 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await page.waitForURL(/^https:\/\/fil\.one\/?$/, { timeout: 30_000 }); | ||
| await expect(page).toHaveURL(/^https:\/\/fil\.one\/?$/); |
There was a problem hiding this comment.
The logout test hard-codes https://fil.one, but the suite is documented to run against deployed environments (e.g. https://staging.fil.one) via BASE_URL. This will fail when baseURL is not the production domain. Prefer asserting against the configured base URL (e.g., derive the expected origin from testInfo.project.use.baseURL or compare new URL(page.url()).origin to the configured origin) while still allowing the Auth0 redirect chain to complete.
| const storagePath = STORAGE_STATE[role.name]; | ||
| await fs.mkdir(path.dirname(storagePath), { recursive: true }); | ||
| await page.context().storageState({ path: storagePath }); | ||
| await page.context().storageState({ path: STORAGE_STATE[role.name] }); |
There was a problem hiding this comment.
The storage state is written twice to the same path (line 49 and 50). This is redundant and can obscure intent; keep a single storageState({ path }) call (using the storagePath variable) to avoid extra I/O and confusion.
| ### E2E Tests | ||
|
|
||
| The repo includes a Playwright end-to-end test suite under `tests/e2e/`. The `@playwright/test` package is already a devDependency, so `pnpm install` covers it. | ||
| Playwright end-to-end test suite under `tests/e2e/` that runs against a deployed environment (typically staging). The `@playwright/test` package is already a devDependency, so `pnpm install` covers it. |
There was a problem hiding this comment.
This sentence is missing a subject (reads like a fragment). Consider rewriting to a complete sentence (e.g., The repo includes a Playwright end-to-end test suite...) for clarity.
| // Convention: one spec per feature, one describe block per role. Each test name | ||
| // starts with the role being exercised so reports read clearly in CI. | ||
|
|
||
| test.describe('paid user', () => { |
There was a problem hiding this comment.
this is a good start but I do think we can do more testing on the dashboard. Especially with regard to activity feed. Not sure how much challenge it is given the account state matters a lot in terms of what buckets/objects/keys exist in the account.
joemocode-business
left a comment
There was a problem hiding this comment.
Really great work! very good foundation to build more tests on.
This pull request introduces an E2E test suite containing basic authentication and bucket operation tests. Currently bucket creation is skipped until the bucket deletion is enabled as we operate under 100 bucket limit.
E2E Test Infrastructure and CI Improvements:
auth.setup.ts) that authenticates each test user and reseeds their billing state in DynamoDB before tests run, ensuring consistent and reliable test conditions. [1]], [2]])e2e-staging.yaml,test-staging.yaml) to securely inject all required E2E credential environment variables and configure AWS credentials via OIDC, enabling direct writes to the stagingBillingTable. [1]], [2]], [3]], [4]])Test Suite Robustness and Configuration:
playwright.config.ts, failing fast if any are missing, and documented the required variables. ([playwright.config.tsR12-R31])sst shell, ensuring SST resource bindings (likeBillingTablename) resolve to the correct stage. ([package.jsonL26-R33])Documentation Enhancements:
README.mdwith detailed explanations of required environment variables, billing state reset logic, local and CI setup, and instructions for adding new roles. [1]], [2]])New and Updated Test Coverage:
Dependency Updates:
@aws-sdk/client-dynamodbas a dev dependency to support direct DynamoDB access for billing state resets in E2E tests. ([pnpm-lock.yamlR18-R20])These changes collectively make E2E tests more reliable, repeatable, and secure, while improving documentation and developer experience.