-
Notifications
You must be signed in to change notification settings - Fork 54
Scope selector support for POA #2077
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for element selector bounding box data (elementSelectorsData/boundingBoxes) in Percy snapshot comparisons, enabling tracking and validation of specific element positions within screenshots for improved visual testing accuracy and debugging.
- Adds
elementSelectorsDatafield to comparison schema and client API - Updates AutomateProvider and GenericProvider to handle
boundingBoxesdata from screenshot operations - Includes comprehensive test coverage for various scenarios (successful lookups, failures, empty objects, multiple selectors)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/core/src/config.js |
Adds elementSelectorsData schema to comparison configuration |
packages/core/test/unit/config.test.js |
Adds validation tests for elementSelectorsData with various coordinate scenarios |
packages/client/src/client.js |
Updates createComparison to accept and pass elementSelectorsData parameter |
packages/client/test/client.test.js |
Adds tests for elementSelectorsData handling in comparison creation |
packages/webdriver-utils/src/providers/genericProvider.js |
Maps boundingBoxes from tiles to elementSelectorsData in screenshot response |
packages/webdriver-utils/src/providers/automateProvider.js |
Extracts bounding_boxes from BrowserStack response |
packages/webdriver-utils/test/providers/genericProvider.test.js |
Tests elementSelectorsData inclusion when boundingBoxes are provided |
packages/webdriver-utils/test/providers/automateProvider.test.js |
Comprehensive tests for bounding box handling in fullpage and singlepage modes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/core/src/config.js
Outdated
| top: { type: 'number' }, | ||
| left: { type: 'number' }, | ||
| bottom: { type: 'number' }, | ||
| right: { type: 'number' }, |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema defines coordinate properties (top, left, bottom, right) as type 'number', but the test case "should handle failed bounding box lookups" shows these properties can be null when success is false (lines 516-519). The schema should allow null values for these coordinate properties by changing their type to ['number', 'null'], similar to how stacktrace is defined.
| top: { type: 'number' }, | |
| left: { type: 'number' }, | |
| bottom: { type: 'number' }, | |
| right: { type: 'number' }, | |
| top: { type: ['number', 'null'] }, | |
| left: { type: ['number', 'null'] }, | |
| bottom: { type: ['number', 'null'] }, | |
| right: { type: ['number', 'null'] }, |
| describe('ComparisonSchema - elementSelectorsData', () => { | ||
| beforeEach(() => { | ||
| PercyConfig.addSchema(CoreConfig.schemas); | ||
| }); | ||
|
|
||
| it('should accept valid elementSelectorsData with xpath selector and coordinates', () => { | ||
| const comparison = { | ||
| name: 'test-snapshot', | ||
| tag: { | ||
| name: 'test-tag', | ||
| width: 1280, | ||
| height: 1024 | ||
| }, | ||
| elementSelectorsData: { | ||
| '//*[@id="__next"]/div/div': { | ||
| success: true, | ||
| top: 0, | ||
| left: 0, | ||
| bottom: 1688.0625, | ||
| right: 1280, | ||
| message: 'Found', | ||
| stacktrace: null | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const errors = PercyConfig.validate(comparison, '/comparison'); | ||
| expect(errors).toBe(undefined); | ||
| }); | ||
|
|
||
| it('should accept elementSelectorsData with multiple selectors', () => { | ||
| const comparison = { | ||
| name: 'test-snapshot', | ||
| tag: { | ||
| name: 'test-tag', | ||
| width: 1280, | ||
| height: 1024 | ||
| }, | ||
| elementSelectorsData: { | ||
| '//*[@id="header"]': { | ||
| success: true, | ||
| top: 0, | ||
| left: 0, | ||
| bottom: 100, | ||
| right: 1280, | ||
| message: 'Found', | ||
| stacktrace: null | ||
| }, | ||
| '//*[@id="footer"]': { | ||
| success: false, | ||
| top: 0, | ||
| left: 0, | ||
| bottom: 0, | ||
| right: 0, | ||
| message: 'Not found', | ||
| stacktrace: 'Element not visible' | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const errors = PercyConfig.validate(comparison, '/comparison'); | ||
| expect(errors).toBe(undefined); | ||
| }); | ||
|
|
||
| it('should accept elementSelectorsData with success false and stacktrace', () => { | ||
| const comparison = { | ||
| name: 'test-snapshot', | ||
| tag: { | ||
| name: 'test-tag', | ||
| width: 1280, | ||
| height: 1024 | ||
| }, | ||
| elementSelectorsData: { | ||
| '//div[@class="missing"]': { | ||
| success: false, | ||
| top: 0, | ||
| left: 0, | ||
| bottom: 0, | ||
| right: 0, | ||
| message: 'Element not found', | ||
| stacktrace: 'Timeout waiting for element' | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const errors = PercyConfig.validate(comparison, '/comparison'); | ||
| expect(errors).toBe(undefined); | ||
| }); | ||
|
|
||
| it('should accept empty elementSelectorsData object', () => { | ||
| const comparison = { | ||
| name: 'test-snapshot', | ||
| tag: { | ||
| name: 'test-tag', | ||
| width: 1280, | ||
| height: 1024 | ||
| }, | ||
| elementSelectorsData: {} | ||
| }; | ||
|
|
||
| const errors = PercyConfig.validate(comparison, '/comparison'); | ||
| expect(errors).toBe(undefined); | ||
| }); | ||
|
|
||
| it('should accept elementSelectorsData with decimal coordinates', () => { | ||
| const comparison = { | ||
| name: 'test-snapshot', | ||
| tag: { | ||
| name: 'test-tag', | ||
| width: 1280, | ||
| height: 1024 | ||
| }, | ||
| elementSelectorsData: { | ||
| '//*[@id="content"]': { | ||
| success: true, | ||
| top: 100.5, | ||
| left: 50.25, | ||
| bottom: 500.75, | ||
| right: 1200.125, | ||
| message: 'Found', | ||
| stacktrace: null | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const errors = PercyConfig.validate(comparison, '/comparison'); | ||
| expect(errors).toBe(undefined); | ||
| }); | ||
|
|
||
| it('should accept elementSelectorsData with negative coordinates', () => { | ||
| const comparison = { | ||
| name: 'test-snapshot', | ||
| tag: { | ||
| name: 'test-tag', | ||
| width: 1280, | ||
| height: 1024 | ||
| }, | ||
| elementSelectorsData: { | ||
| '//*[@id="offscreen"]': { | ||
| success: true, | ||
| top: -100, | ||
| left: -50, | ||
| bottom: 0, | ||
| right: 0, | ||
| message: 'Found off-screen', | ||
| stacktrace: null | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const errors = PercyConfig.validate(comparison, '/comparison'); | ||
| expect(errors).toBe(undefined); | ||
| }); |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test case validating that elementSelectorsData with null coordinate values (which occurs when success is false) passes schema validation. Based on the automateProvider test showing null coordinates for failed lookups, a test case should be added to verify the schema accepts this structure.
| it('should handle empty bounding boxes object', async () => { | ||
| const response = { | ||
| success: true, | ||
| result: JSON.stringify({ | ||
| tiles: [{ | ||
| sha: 'abc', | ||
| status_bar: 0, | ||
| nav_bar: 156, | ||
| header_height: 0, | ||
| footer_height: 156, | ||
| index: 0 | ||
| }], | ||
| dom_sha: 'def', | ||
| bounding_boxes: {} | ||
| }) | ||
| }; | ||
| browserstackExecutorSpy = spyOn(AutomateProvider.prototype, 'browserstackExecutor') | ||
| .and.returnValue(Promise.resolve({ value: JSON.stringify(response) })); | ||
| await automateProvider.createDriver(); | ||
| const res = await automateProvider.getTiles(false); | ||
| const expectedOutput = { | ||
| tiles: [ | ||
| new Tile({ | ||
| statusBarHeight: 0, | ||
| navBarHeight: 156, | ||
| headerHeight: 0, | ||
| footerHeight: 156, | ||
| fullscreen: false, | ||
| sha: 'abc' | ||
| }) | ||
| ], | ||
| domInfoSha: 'def', | ||
| metadata: { | ||
| screenshotType: 'singlepage' | ||
| }, | ||
| boundingBoxes: {} | ||
| }; | ||
| expect(browserstackExecutorSpy).toHaveBeenCalledTimes(1); | ||
| expect(executeScriptSpy).toHaveBeenCalledTimes(1); | ||
| expect(res).toEqual(expectedOutput); | ||
| }); |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test expects an empty bounding boxes object {} to be preserved (line 715), but the implementation in automateProvider.js line 103 uses tileResponse.bounding_boxes || null, which would return null for undefined but preserve empty objects. Consider explicitly documenting this behavior or add a check to normalize empty objects to null for consistency with how missing data is handled elsewhere (e.g., when bounding_boxes is not present in the response).
| elementSelectorsData: { | ||
| type: 'object', | ||
| additionalProperties: { | ||
| type: 'object', | ||
| properties: { | ||
| success: { type: 'boolean' }, | ||
| top: { type: 'number' }, | ||
| left: { type: 'number' }, | ||
| bottom: { type: 'number' }, | ||
| right: { type: 'number' }, | ||
| message: { type: 'string' }, | ||
| stacktrace: { type: ['string', 'null'] } | ||
| } | ||
| } |
Copilot
AI
Dec 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema for elementSelectorsData doesn't specify which properties are required. Based on the test cases and actual usage, all properties (success, top, left, bottom, right, message, stacktrace) should be present in every element selector object. Consider adding a required array to ensure data consistency and prevent incomplete data from being accepted.
This pull request adds support for including element selector bounding box data (called
elementSelectorsDataorboundingBoxes) in Percy snapshot comparisons. This allows the system to track and validate the positions and statuses of specific elements within screenshots, improving debugging and accuracy for visual testing. The changes include updates to the client, core configuration, WebDriver utilities, and comprehensive tests to ensure correct handling of this new data.