test: e2e playwright implementation#1389
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive developer documentation bundle under the developer-docs/ directory, adhering to the Open Knowledge Format (OKF) v0.1. It includes a documentation policy, architectural decisions (AD-1 to AD-5), an examples inventory, a testing strategy, a playbook for dependency updates, and a work queue for Playwright e2e smoke tests. It also updates root files like .gitignore, AGENTS.md, CONTRIBUTING.md, and LOCAL_DEVELOPMENT.md to reference this new bundle. The review feedback correctly identifies that multiple internal markdown links use absolute paths starting with / (e.g., /decisions.md), which will break when rendered on GitHub or in local markdown viewers. These should be updated to use relative paths as suggested.
5c80e84 to
54d2d62
Compare
54d2d62 to
266423f
Compare
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
409e138 to
046d91d
Compare
046d91d to
04530b9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
0b55d65 to
93911c1
Compare
Wire non-browser boot smoke for the Express example (AD-6): ui|http example kind, serial runner entry, and spec-managed lifecycle on :4001. Update testing-strategy and AD-6 as durable owners; defer Phase 5 auth flags (no flakiness observed).
CI (e2e.yaml): path filters for scripts/** and firebase.json; frozen lockfile; pnpm/action-setup before setup-node so store cache restores; Playwright browser cache without restore-keys; Auth emulator started by Playwright globalSetup; HTML/JSON/coverage artifacts uploaded every run. E2E runtime: webServer timeouts 240s (angular) and 180s (nextjs); global expect.timeout 30s; serial emulator via E2E_KEEP_EMULATOR and runner postflight; e2e package test runs full serial runner; S3 asserts per-example forgot-password route; custom-auth-server spawn node build/index.js directly (see package.json start) to avoid pnpm SIGTERM noise on teardown. Emulator config: e2e/firebase.emulator.json is auth-only — no global webframeworks experiment mutation during pnpm test:e2e. Coverage: V8 precise coverage summary uses weightedExecUnits (byte-range × count); shared reset in e2e/fixtures/coverage-artifacts.mjs; raw.jsonl compresses well in upload-artifact zip. Toolchain: firebase-tools version parsed from test.yaml at e2e globalSetup; root packageManager pnpm@10.34.4 (Phase 6 bump target). Lockfile: exclude @angular/fire optional firebase-tools peer via pnpm-workspace overrides; emulator tooling remains npx/global CLI; remove stray examples/nextjs-ssr/package-lock.json. Docs: LOCAL_DEVELOPMENT and dependency playbook document test:e2e; AD-4 top-level webServer; AD-7 broad path triggers; examples-inventory owns ports/commands; testing-strategy reflects globalSetup emulator; documentation-policy root links fixed; remove session log and redundant OKF index stubs per documentation-policy. CI: correct upload-artifact SHA (v4.6.1); pin pnpm 10.34.4 in e2e/lint/test workflows to match packageManager; e2e path triggers include test.yaml. E2E: S3 asserts rendered forgot-password UI only (no URL check per AD-3); globalSetup spawns firebase-tools via npx argv (shell: false); strict TS for firebase-tools version parse; V8 weightedExecUnits skips unexecuted ranges.
74365f3 to
483d996
Compare
| deps: [FirebaseApps, [new Optional(), Auth]], | ||
| useFactory: () => { | ||
| const apps = inject(FirebaseApps); | ||
| useFactory: (apps: FirebaseApps) => { |
There was a problem hiding this comment.
This is an important difference that needs a look. This seemed like the cleanest way to get the dependency injected in the API without fancy tricks. We're still in pre-release so the break between previous style should be okay?
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 |
There was a problem hiding this comment.
wiz flags a file if it has problems and you touch it - I touched this (and tset.yaml) to pin the pnpm version, so I had to then pin all the SHAs for actions in order to satisfy wiz
| @@ -0,0 +1,59 @@ | |||
| --- | |||
| type: Decision Log | |||
| title: Architecture decisions | |||
There was a problem hiding this comment.
These can be changed at any time, but imply a refactor cycle (and possibly a semver major for API differences) if they are changed.
Recording architecture decisions that become fundamental assumptions in the code is best done overtly rather than by code implication though, hopefully this format works
| timestamp: string; | ||
| }; | ||
|
|
||
| export async function startV8Coverage(page: Page): Promise<CDPSession | null> { |
There was a problem hiding this comment.
lcov is more useful but this PR is large enough, deferred for later.
| @@ -0,0 +1,8 @@ | |||
| { | |||
There was a problem hiding this comment.
this allows for the experimental web feature support in the emulators to be turned on in an e2e / ci focused way by using a config here, instead of turning it on in a developer's host machine global config (which is a bit presumptious)
|
|
||
| return ( | ||
| <SignInAuthScreen | ||
| onForgotPasswordClick={() => { |
There was a problem hiding this comment.
this was needed for parity with the other examples so we could generically route in all of them to fogot password during e2e smoke
Implements Playwright e2e smoke tests for every monorepo example (five UI examples +
custom-auth-serverHTTP smoke), runnable locally viapnpm test:e2eand in CI.Start with developer-docs/decisions.md — the AD log is the most important part of this PR to review. In particular, please scrutinize the Angular dependency-management changes that unblock the angular-example smoke (they have the largest non-e2e impact):
@firebase-oss/ui-angularprovideFirebaseUIfactory now receivesFirebaseAppsvia DIdepsinstead of callinginject()inside the factory (packages/angular/src/lib/provider.ts)angular-examplemarks@firebase-oss/ui-angularas a pnpm injected workspace dependency so Angular resolves peers correctly at build/serve time (examples/angular/package.json)pnpm-workspace.yamlblocks@angular/fire's optionalfirebase-toolspeer from entering the lockfile (e2e usesnpx; CI uses global install) — see AD-4 emulator tooling notes and the work-queue Phase 2b rationale in playwright-e2e-smoke.mdAlso worth a pass: AD-3 (MVP scope), AD-4 (serial runner + shared emulator), AD-6 (
custom-auth-serveron:4001), AD-7 (separate e2e workflow + broad triggers), AD-8 (production-artifact e2e deferred).What's included
Documentation (
developer-docs/OKF bundle)AGENTS.mdentry point; links fromCONTRIBUTING.mdandLOCAL_DEVELOPMENT.mdE2E framework (
e2e/,scripts/e2e-run.mjs)@playwright/testpinned via pnpm catalogpnpm test:e2e) and per-example debug scripts (pnpm test:e2e:<example>)globalSetup/globalTeardown:build:packages, auth-only emulator config, shared Auth emulator on:9099custom-auth-serverHTTP boot smoke on:4001page.route; interim V8 coverage artifacts (best-effort, not a gate)Example / package fixes
examples/nextjs-ssr/package-lock.jsonCI
.github/workflows/e2e.yaml— Chromium, Playwright browser cache, artifact uploadtest.yaml/lint.yaml: pnpm10.34.4, pinned action SHAs,contents: readpermissions (zizmor)Deferred (documented, not in this PR)
Test plan
pnpm test:e2egreen locally