[PM-42193] introduce autofill trigger tests - #526
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new opt-in Code Review Details
|
Claude Code validationResult: Issues found (no blocking errors — 4 minor warnings) Validated the one Claude-material change in this PR, CriticalNone. MajorNone. Minor
Note, outside this diff: Checks run
Secret scan on the changed file found only pre-existing prose in the unchanged "Security |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| await fillBadge.or(fillHover).first().click(); | ||
|
|
||
| // The popup click autofilled the web form. | ||
| await expectInputsAutofilled(formPage, inputs); |
There was a problem hiding this comment.
Details and fix
For the three /forms/search/* pages every input is shouldNotAutofill, so expectInputsAutofilled asserts toHaveValue("") on fields that are already empty — it matches on the first poll. The popup → background → content-script fill is asynchronous, so those pages pass without ever proving the fill was suppressed.
autofill-forms.spec.ts guards exactly this ("when expecting an empty value, the test may pass before the fill, potentially resulting in a false positive"), and page-load-autofill.spec.ts uses onLoadFillSettleDelay for the same reason.
await fillBadge.or(fillHover).first().click();
// Give the popup-triggered fill a moment before reading values, so pages that
// expect NO fill can't pass before the fill would have landed.
await formPage.waitForTimeout(popupFillSettleDelay);
// The popup click autofilled the web form.
await expectInputsAutofilled(formPage, inputs);Pages with at least one expected-filled input do self-synchronize (the retrying assertion on the first key waits for the fill), so only the all-empty pages are affected.
| // The iframe embeds the bare payment page; match that URI too so the card | ||
| // cipher is offered inside the frame (mirrors the iframe-login cipher). | ||
| additionalLoginUrls: [`${testSiteHost}/payment-page-bare`], | ||
| uriMatchType: UriMatchType.StartsWith, |
There was a problem hiding this comment.
♻️ DEBT: additionalLoginUrls has no effect on a Card cipher, and the comment claims it does.
Details and fix
scripts/vault-seeder.ts reads additionalLoginUrls (and uriMatchType) only inside generateLoginItemData, which returns null when cipherType !== CipherType.Login. Card items are built by generateCardItemData, which emits no URIs at all — so nothing here reaches the seeded item, and the card is matched by type rather than URI.
The comment asserting the frame URI is matched will send the next person debugging iframe card fill down the wrong path. Suggest dropping the two lines (the existing card-payment entry's uriMatchType is equally inert) or replacing the comment with a note that card ciphers are offered by cipher type, not URI match.
| /** | ||
| Which cipher type autofill fills for a page. | ||
| */ | ||
| export const AutofillCommand = { | ||
| /** Fill the matching login cipher. */ | ||
| Login: "login", | ||
| /** Fill the matching card cipher. */ | ||
| Card: "card", | ||
| /** Fill the matching identity cipher. */ | ||
| Identity: "identity", | ||
| } as const; |
There was a problem hiding this comment.
♻️ DEBT: Runtime value placed in the types-only abstractions layer creates a module cycle with constants.
Details and fix
Before this change abstractions/ exported types exclusively, and every enum-like value object in the repo lives in constants/ (TestNames, CipherType, UriMatchType). constants/test-pages.ts now imports AutofillCommand as a value from ../abstractions, while abstractions/test-pages.ts and abstractions/vault-seeder.ts import from ../constants — a source-level cycle.
It is benign today only because those constants imports are used in type positions and TS elides them from the emitted CommonJS. The first value use of TestNames/CipherType/UriMatchType inside abstractions/ makes it a real require cycle, where AutofillCommand can be undefined while constants/test-pages.ts initializes.
Moving AutofillCommand to constants/test-pages.ts (beside TestNames) preserves the layering and removes the cycle; AutofillCommandValue can stay a type here.
| TestNames.InlineMenuAutofill, // @TODO known failure - need to update test design to handle this test page case (e.g. existing ciphers should appear for password input, any existing identity ciphers for new email input) | ||
| TestNames.MessageAutofill, // @TODO known failure - fills new email input with existing email (PM-26477) | ||
| TestNames.PageLoadAutofill, // @TODO known failure - message-based fill wrongly fills the new-email input with the existing email (PM-26477) | ||
| TestNames.PopupAutofill, // @TODO known failure - message-based fill wrongly fills the new-email input with the existing email (PM-26477) |
There was a problem hiding this comment.
❓ QUESTION: Should /forms/update/update-password (next entry) also skip PopupAutofill?
Reasoning
That page skips MessageAutofill because the command-triggered fill populates the autocomplete="new-password" inputs (PM-26477), while newPassword/newPasswordRetype expect "". Popup-triggered fill is also an explicit, user-initiated fill (new-password filling enabled), unlike page-load fill — so I'd expect the same failure mode there. Did the trigger suite pass on that page locally?
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-42193
📔 Objective
Add tests confirming additional trigger conditions; increase BIT coverage for clients#22380.