[quexten]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-45737

📔 Objective

Add a writing-end-to-end-tests skill for Playwright tests in e2e/:

• Locate elements only by test id. Add missing data-testids to templates in the same change.
• Run the written tests and report the result; bug-fix tests must also fail without the fix.
• Points to existing fixtures, page helpers and the automation driver.

📸 Screenshots

n/a

[github-actions]

Claude Code validation

Result: Pass

Re-reviewed .claude/skills/writing-end-to-end-tests/SKILL.md after the fixes pushed since the last run. All three IMPORTANT findings are genuinely resolved: the description now scopes the skill to the key-management experiment and agrees with e2e/README.md:3-5, the locator rule is scoped to new locators with an explicit exemption for the existing helpers, and the bash block carries the xvfb-run note for the three non-headless suites. Frontmatter parses cleanly — the earlier experiment: use colon-space report is fixed, and line 3 now contains no colon at all. Six SUGGESTED accuracy items remain, none blocking.

One correction to the previous report, which this comment replaces: its fix for Finding 5 pointed at the module filenames in libs/automation-driver/src/capabilities/. Those are kebab-case and are not the strings automationDriver accepts. The accepted values are the automationName fields, which are camelCase. Finding 2 below carries the right list.

Not covered: The configuration and security review covers no file in this changeset. The only changed path is a SKILL.md, which that review excludes by design so it does not duplicate the skill review. No plugin changed, so plugin validation and the structure, marketplace and version-bump scripts did not apply.

Findings

• 🎨 SUGGESTED: Finding 1: automationDriver(page, "<capability>") is shown without its type parameter, so the example resolves to no callable members (.claude/skills/writing-end-to-end-tests/SKILL.md:11)

  Details and fix

  The signature is export function automationDriver<T>(page: Page, capability: string): RemoteCapability<T> (e2e/src/automation-driver.ts:19). RemoteCapability<T> is a mapped type over keyof T, so with T inferred as unknown and no contextually typed assignment target the proxy exposes nothing to call. Real call sites get T from a declared field type (e2e/src/desktop/biometrics.ts:44-49).

  Use the form from the function's own doc comment (e2e/src/automation-driver.ts:15):

  const lock = automationDriver<{ lock(): Promise<void> }>(page, "lock");

• 🎨 SUGGESTED: Finding 2: No pointer to the valid capability strings, so an agent has to guess the second argument (.claude/skills/writing-end-to-end-tests/SKILL.md:11)

  Details and fix

  capability is an untyped string, so a wrong value fails at runtime rather than compile time. Line 11 names "feature flags, lock state, fake biometrics" in prose but never says where the literal keys live.

  The accepted values are the automationName fields, which the service keys on (libs/automation-driver/src/automation-driver.service.ts:23): biometrics, lock, featureFlags, state, logging, desktopNavigation, sdk, processReload. Note these are camelCase and differ from the kebab-case filenames in that directory — feature-flags.ts declares automationName = "featureFlags" (libs/automation-driver/src/capabilities/feature-flags.ts:12), so an agent that reads filenames gets three of the eight wrong.

  Append to line 11: "capability names are the automationName values declared in libs/automation-driver/src/capabilities/, not the filenames".

• 🎨 SUGGESTED: Finding 3: "shared holds flows spanning clients" understates the shared suite and omits which fixture it uses (.claude/skills/writing-end-to-end-tests/SKILL.md:8)

  Details and fix

  playwright.shared.config.ts:6-9 describes running the web vault next to the browser extension in one persistent Chromium context, and e2e/tests/shared/passkey-unlock.e2e.ts:2-9 imports test/expect from ../../src/browser/fixtures. Desktop is not part of it. Line 9 lists only the browser and desktop fixtures and says web uses page, leaving shared unaccounted for, and that fixture import is not guessable from the current wording.

  Reword line 8 to: "shared runs the web vault and the extension together in one Chromium context, on e2e/src/browser/fixtures.ts".

• 🎨 SUGGESTED: Finding 4: "Rebuild the client first if its code changed" names no command (.claude/skills/writing-end-to-end-tests/SKILL.md:43)

  Details and fix

  The npx playwright test path deliberately skips the build steps baked into the npm scripts, which is exactly when this sentence matters — an agent that follows it runs against a stale build with no way to refresh it. The commands are in package.json:27-29:

  npm run build:prod:chrome --workspace @bitwarden/browser   # browser and shared
  npm run build-native --workspace @bitwarden/desktop && \
    npm run build:dev --workspace @bitwarden/desktop         # desktop

• 🎨 SUGGESTED: Finding 5: The failure-artifact list omits video and how to open a trace (.claude/skills/writing-end-to-end-tests/SKILL.md:45)

  Details and fix

  The directory is right (outputDir: "test-results" in e2e/playwright.base.ts:17), but trace.zip is not readable as a file, so an agent told to "read" it will try and fail. e2e/README.md:26-27 documents both the viewer and the video, and e2e/src/artifacts.ts:9,13,24-26 confirms a failing test's video lands in a video/ subdirectory of the same output dir.

  Extend the list to: (screenshot, error-context.md, video/, trace.zip — open it with npx playwright show-trace <trace.zip>). E2E_VIDEO=1 records video for passing tests too.

• 🎨 SUGGESTED: Finding 6: test:e2e:shared builds the extension but is the one line in the block with no build comment (.claude/skills/writing-end-to-end-tests/SKILL.md:37)

  Details and fix

  Lines 35-36 annotate their builds, so the unannotated line 37 reads as cheap and web-only by contrast. package.json:29 shows test:e2e:shared runs npm run build:prod:chrome --workspace @bitwarden/browser first, the same as test:e2e:browser.

  Add the matching comment to line 37:

  npm run test:e2e:shared    # builds the extension first

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds one new file, .claude/skills/writing-end-to-end-tests/SKILL.md, a project skill with guidance for writing and running the Playwright tests in e2e/. Every path, helper, and command it names exists in the repository: the fixtures, automationDriver, LoginPage/LockScreen, the *.e2e.ts test match, the test:e2e:* scripts, the playwright.<suite>.config.ts files, and the test-results/ trace output. No credentials or sensitive material were found in the file.

Not covered: Skill review did not run. This review path cannot launch plugin-dev:skill-reviewer, so .claude/skills/writing-end-to-end-tests/SKILL.md was not checked for description quality, length, or progressive disclosure. performing-multi-agent-code-review covers those checks where plugin-dev is installed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.23%. Comparing base (dfcc199) to head (2eb9fc5).
⚠️ Report is 25 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24151      +/-   ##
==========================================
+ Coverage   59.82%   60.23%   +0.40%     
==========================================
  Files        4723     4776      +53     
  Lines      149790   151810    +2020     
  Branches    23909    24248     +339     
==========================================
+ Hits        89615    91436    +1821     
- Misses      53947    54066     +119     
- Partials     6228     6308      +80     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[SaintPatrck]

FYI @kdenney and @awiester-bw. I know you've been working on testing components. Might be worth collaborating with @quexten here to make sure we're not duplicating work or creating competing components.

[quexten]

@SaintPatrck Currently, this skill covers writing playwright e2e tests within the clients repo, which are owned by KM, and very repo-specific. I don't see how another team would govern the skills to write code specific to km-owned e2e tests?

The testing tools:
bitwarden/ai-plugins#282 are entirely orthogonal to this current effort, because they are 1. non-repos specific, and 2. do not govern how to implement tests, but rather govern which tests to implement.

Is there any other effort you are referring to?

[quexten]

@SaintPatrck Okay, after looking over it, the related effort https://github.com/bitwarden/ai-plugins/pull/212/changes#diff-243aa645e4b979d8704b8f196512b0ea47e03a67197f454850716c12cceda153 seems also orthogonal. That one ad-hoc assembles based on natural language a playwright script meant to run once. The repo's e2e tests are meant to be stable tests of behavior, not one-time validations.

[quexten]

Something i'm not sure on; should we give concrete references here? Like: "If you want to emulate biometrics, use X" etc?

[SaintPatrck]

💭 My general rule of thumb is to provide concrete references and detailed explanations or reasoning only after you observe the agent struggling.

Frontloading excess information without proof it's necessary often results in bloat that complicates ablation later, as the models improve.

When you feel concrete references are necessary, they're best isolated in reference/ files, to take advantage of progressive disclosure.

[quexten]

Makes sense, we'll look into it as we use it then, thank you!

[kdenney]

Agree with @quexten that this is orthogonal to the playwright testing skills I'm working on. Removed myself from reviewers to not hold up this pr.

[awiester-bw]

Also removing as this is orthogonal to PR282.