browser-extension
browser-extension copied to clipboard
rainbow-me
chore: percy visual testing
Fixes BX-#### Figma link (if any):
What changed (plus any additional context for devs)
Screen recordings / screenshots
What to test
PR-Codex overview
This PR focuses on enhancing end-to-end (E2E) testing capabilities and improving code organization by refactoring test structures, updating file paths, and adding configuration for better test management.
Detailed summary
- Updated
.percy.ymlfor file uploads. - Refactored E2E test files to use
rootURLanddrivercontext in tests. - Changed mock file paths in
mockFetch.tsandupdateQuotes.ts. - Modified test cases to use async/await with context parameters.
- Added global setup in
setup.tsfor E2E tests. - Improved documentation in
CLAUDE.mdregarding E2E testing. - Enhanced
FullScreenBackground.tsxwithdata-viewportattributes. - Updated various test files to improve clarity and consistency in naming and structure.
The following files were skipped due to too many changes:
e2e/parallel/nftsFlow.test.ts,e2e/parallel/shortcuts-home.test.ts,e2e/util/snapshot.ts,e2e/serial/dappInteractions/1_appInteractionsFlow.test.ts,e2e/serial/swap/3_shortcuts-swapFlow.test.ts,e2e/serial/send/2_shortcuts-sendFlow.test.ts,.github/workflows/build.yml,e2e/serial/send/1_sendFlow.test.ts,e2e/serial/dappInteractions/3_dappAccountsSwitcher.test.ts,e2e/serial/swap/2_swapFlow2.test.ts,e2e/serial/dappInteractions/2_dappInteractionFlow.test.ts,e2e/parallel/shortcuts-settings.test.ts,e2e/serial/swap/1_swapFlow1.test.ts,yarn.lock
✨ Ask PR-Codex anything about this PR by commenting with
/codex {your question}
⚠️ No Changeset found
Latest commit: a8a753e06a807a48e733decce9841ec591ff9765
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
![changeset-bot[bot] avatar](https://avatars.githubusercontent.com/in/28616?v=4 "Published by a pub.dev verified publisher"){kind=small}
Claude finished @DanielSinclair's task —— View job
Code Review
Review Checklist
- [x] Review Percy configuration and workflow changes
- [x] Examine E2E test refactoring patterns
- [x] Check snapshot utility implementation
- [x] Review mock file reorganization
- [x] Assess TypeScript safety
- [x] Check for performance considerations
- [x] Verify changeset requirements
Issues Found
1. Hard-coded Device Pixel Ratio
In e2e/util/snapshot.ts:186-188, the device pixel ratio is hard-coded to 2:
const dpr = 2;
const cropWidth = targetWidth * dpr;
const cropHeight = targetHeight * dpr;
This assumes all screenshots are taken on Retina displays, which may cause incorrect cropping on non-Retina systems. Consider making this configurable or detecting it from the environment.
2. Missing Error Handling in Global Setup
The e2e/setup.ts file doesn't handle potential failures in Percy screenshot capture gracefully:
beforeEach(async (context) => {
context.driver = globalDriver;
context.rootURL = globalRootURL;
if (context.driver) {
await captureSnapshot(context); // Could fail and break test setup
}
});
Consider wrapping in try-catch to prevent test setup failures from blocking test execution.
3. Type Safety Issues
The snapshot utility uses require instead of ES modules:
const sharp = require('sharp'); // Line 6
This bypasses TypeScript's type checking. Consider using:
import sharp from 'sharp';
4. Potential Race Condition in Screenshot Naming
The duplicate filename handling in generateScreenshotFilename has a potential race condition:
while (fs.existsSync(`screenshots/${finalFileName}.png`)) {
counter += 1;
finalFileName = `${fileName}_${counter}`;
if (counter > 10) break;
}
If multiple tests run in parallel and generate screenshots simultaneously, they could overwrite each other.
5. Missing Changeset
This PR adds significant new functionality (Percy visual testing) but doesn't include a changeset. While this is infrastructure code, consider adding a changeset to document the addition of visual regression testing capabilities.
6. Inconsistent Error Handling
The mockFetch function throws errors inconsistently:
if (!response)
throw new Error('no response for request', {
cause: { url: url.href, hash },
});
But the snapshot utility only logs warnings:
} catch (error) {
console.warn(`Failed to capture snapshot: ${error}`);
}
Consider standardizing error handling approach across the E2E utilities.
7. Percy Configuration Security
The Percy token is exposed in multiple workflow files. While it's using secrets, consider consolidating Percy configuration to reduce the number of places where tokens need to be configured.
Recommendations
- Make DPR configurable: Add an environment variable or detect from the browser capabilities
- Add proper error boundaries: Ensure screenshot failures don't break test execution
- Use ES modules consistently: Replace
requirewith proper imports - Implement file locking: Use a more robust approach for unique filenames
- Add a changeset: Document the addition of visual testing capabilities
- Standardize error handling: Create consistent patterns for E2E utilities
- Consider Percy parallelization: The
PERCY_PARALLEL_TOTAL: -1setting might need tuning for optimal performance
![claude[bot] avatar](https://avatars.githubusercontent.com/in/1236702?v=4 "Published by a pub.dev verified publisher"){kind=small}
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
![socket-security[bot] avatar](https://avatars.githubusercontent.com/in/156372?v=4 "Published by a pub.dev verified publisher"){kind=small}