chore: percy visual testing #1970
Conversation
|
|
Claude finished @DanielSinclair's task —— View job Code ReviewReview Checklist
Issues Found1. Hard-coded Device Pixel RatioIn 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 SetupThe 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 IssuesThe snapshot utility uses const sharp = require('sharp'); // Line 6This bypasses TypeScript's type checking. Consider using: import sharp from 'sharp';4. Potential Race Condition in Screenshot NamingThe duplicate filename handling in 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 ChangesetThis 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 HandlingThe 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 SecurityThe 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
|
DanielSinclair
force-pushed
the
@daniel/percy-e2e
branch
4 times, most recently
from
1a0e68d to
1d38b1b
Compare
DanielSinclair
force-pushed
the
@daniel/percy-e2e
branch
2 times, most recently
from
3cfefee to
209e629
Compare
This reverts commit 98b5c2c.
DanielSinclair
force-pushed
the
@daniel/percy-e2e
branch
from
56072a6 to
4e5c484
Compare
1 participant
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
.percy.ymlfor file uploads.rootURLanddrivercontext in tests.mockFetch.tsandupdateQuotes.ts.setup.tsfor E2E tests.CLAUDE.mdregarding E2E testing.FullScreenBackground.tsxwithdata-viewportattributes.