Proposed changes to e2e helpers#1041
Conversation
gabrielseco
changed the base branch from
main
to
test/pbyr-3802-e2e-oboarding-scenario
📦 Bundle Size Report
Size Limits
Largest Files (Top 5)
View All Files (312 total)
✅ Bundle size check passed |
📊 Coverage Report⚪ Coverage unchanged
Detailed BreakdownLines Coverage
Statements Coverage
Functions Coverage
Branches Coverage
✅ Coverage check passed |
|
Deploy preview for remote-flows ready!
Deployed with vercel-action |
| export type FillFormOptions = { | ||
| type: inputType; | ||
| value?: string; | ||
| cssId?: string; |
There was a problem hiding this comment.
cssId its not longer needed
| value?: string; | ||
| cssId?: string; | ||
| dataField?: string; | ||
| name?: string; |
There was a problem hiding this comment.
name is better in my opinion that data-field as input have the attribute name
| .locator(`[data-field="${dataField}"] :is(input, textarea)`) | ||
| .fill(value); | ||
| } | ||
| await page.locator(`[data-field="${name}"] :is(input, textarea)`).fill(value); |
There was a problem hiding this comment.
we can simplify the code to be in one line and remove all the ifs
| if (dataField) { | ||
| const dropdown = page.locator(`[data-field="${dataField}"]`); | ||
| if (options.nativeSelect) { | ||
| const dropdown = page.locator(`[data-field="${name}"] select`); |
There was a problem hiding this comment.
as the select in the first form provided is different we can something like this
| { type: 'select', value: 'employee', cssId: '#type' }, | ||
| { type: 'textField', value: options.employment_id, cssId: '#employmentId' }, | ||
| { type: 'textField', value: options.external_id, cssId: '#externalId' }, | ||
| { type: 'textField', value: options.company_id, name: 'companyId' }, |
There was a problem hiding this comment.
the API is simplified to use name always
| return ( | ||
| <form onSubmit={handleSubmit} className='onboarding-form-container'> | ||
| <div className='onboarding-form-group'> | ||
| <div data-field='companyId' className='onboarding-form-group'> |
There was a problem hiding this comment.
we make some changes in this form and that way we can fill inputs on the same way
jordividaller
merged commit 604fb96
into
test/pbyr-3802-e2e-oboarding-scenario
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit fd74540. Configure here.
|
|
||
| if (dataField) { | ||
| await page | ||
| .locator(`[data-field="${dataField}"] :is(input, textarea)`) |
There was a problem hiding this comment.
Inconsistent parameter ordering between fill helper functions
Low Severity
fillTextField takes (page, name, value) while every other helper — fillSelect, fillComboBox, fillRadio, fillCheckbox, fillDatepicker — takes (page, value, name/locator). The call sites in fillForm happen to match each signature, so there's no active bug, but this inconsistent ordering across sibling functions in the same file is error-prone for anyone adding new call sites or refactoring.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fd74540. Configure here.
2 participants
Note
Medium Risk
Medium risk because it changes core e2e form-filling helpers and selectors; tests may start failing if any form fields lack the expected
data-fieldwrappers or if select behavior differs between native/custom dropdowns.Overview
Refactors the Playwright
fillFormhelpers to locate inputs consistently via a requirednamemapped to[data-field="..."], removing priorcssId/dataFieldlocator options and adding clearer errors whennameis missing.Adds
nativeSelectsupport tofillSelect(using direct<select>selection vs role-based option clicking) and updates onboarding e2e helpers accordingly. The onboarding intro form UI is also updated to adddata-fieldattributes around its inputs/select so the new locators work.Reviewed by Cursor Bugbot for commit fd74540. Bugbot is set up for automated code reviews on this repo. Configure here.