Tests support. #10
Tests support. #10
Conversation
WalkthroughThis pull request migrates the repository from Node.js/PNPM to Bun (CI workflows, Dockerfile, and packageManager), adds .dockerignore and updates .gitignore, updates GitHub Actions (check, lint, adds test workflow), refactors Playwright handlers in src/routes (reports, screenshots, test) to ensure BrowserContext cleanup and add permissions/header routing, adjusts src/config port parsing, updates package.json scripts, and adds comprehensive unit and end-to-end tests under tests/unit and tests/e2e. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45-60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
19-24: Add--no-sandboxflag to Chromium launch configuration insrc/config/browser.ts(lines 13-16).The review comment is correct: removing
/usr/lib/chromium/chrome-sandboxin the Dockerfile (line 24) without adding the--no-sandboxflag to the Playwright launch args will cause Chromium to fail at startup. When Chromium attempts to use the sandbox and the binary is missing, it will crash with a file-not-found error.Update
src/config/browser.tsline 14 to include the flag:args: ["--remote-debugging-port=9222", "--no-sandbox"],Alternatively, revert the removal of chrome-sandbox from the Dockerfile if sandbox security is preferred.
🧹 Nitpick comments (4)
.dockerignore (1)
9-13: Consider redundancy and LICENSE implications.The pattern
*.mdon line 10 already coversREADME.mdandCHANGELOG.mdlisted on lines 11-12, making those entries redundant.Additionally, excluding
LICENSE(line 13) from the Docker image may have legal compliance implications depending on your licensing requirements. Verify whether the LICENSE file should be included in the final image for proper license attribution.Apply this diff to remove redundant entries:
# Documentation *.md -README.md -CHANGELOG.md LICENSE.github/workflows/check.yml (1)
16-19: Consider pinning the Bun version for reproducibility.Using
bun-version: 'latest'may lead to non-reproducible builds if Bun releases introduce breaking changes or behavioral differences. Consider pinning to a specific version like'1.3.2'to match your Dockerfile for consistency.- name: Setup Bun uses: oven-sh/setup-bun@v1 with: - bun-version: 'latest' + bun-version: '1.3.2'.github/workflows/lint.yml (1)
16-19: Consider pinning the Bun version for reproducibility.Using
bun-version: 'latest'may lead to non-reproducible builds. Pin to a specific version like'1.3.2'to match your Dockerfile for consistency across environments.- name: Setup Bun uses: oven-sh/setup-bun@v1 with: - bun-version: 'latest' + bun-version: '1.3.2'src/server.ts (1)
9-35: LGTM! Consider a routing abstraction for future scalability.The current if-statement routing approach is clear and works well for the four endpoints. As the API grows, consider introducing a route map or framework-based router to improve maintainability.
Example optional refactor using a route map:
const routes = new Map([ ["POST /v1/screenshots", handleScreenshotsRequest], ["POST /v1/reports", handleReportsRequest], ["GET /v1/health", handleHealthRequest], ["GET /v1/test", handleTestRequest], ]); const server = Bun.serve({ port, async fetch(req) { const url = new URL(req.url); const key = `${req.method} ${url.pathname}`; const handler = routes.get(key); if (handler) { return await handler(req); } return new Response("Not Found", { status: 404 }); }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.dockerignore(1 hunks).github/workflows/check.yml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks)Dockerfile(3 hunks)eslint.config.js(1 hunks)package.json(2 hunks)src/config/browser.ts(1 hunks)src/config/index.ts(1 hunks)src/config/lighthouse.ts(1 hunks)src/routes/health.ts(1 hunks)src/routes/index.ts(1 hunks)src/routes/reports.ts(1 hunks)src/routes/screenshots.ts(1 hunks)src/routes/test.ts(1 hunks)src/schemas/index.ts(1 hunks)src/schemas/lighthouse.schema.ts(1 hunks)src/schemas/screenshot.schema.ts(1 hunks)src/server.ts(1 hunks)src/utils/test-page.ts(6 hunks)tests/e2e/endpoints.test.ts(1 hunks)tests/unit/lighthouse.schema.test.ts(1 hunks)tests/unit/screenshot.schema.test.ts(1 hunks)tests/unit/test-page.test.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
tests/unit/test-page.test.ts (1)
src/utils/test-page.ts (1)
generateTestHTML(1-326)
src/routes/health.ts (1)
src/config/browser.ts (1)
browser(13-16)
tests/unit/lighthouse.schema.test.ts (1)
src/schemas/lighthouse.schema.ts (1)
lighthouseSchema(3-73)
src/server.ts (4)
src/routes/screenshots.ts (1)
handleScreenshotsRequest(8-95)src/routes/reports.ts (1)
handleReportsRequest(6-89)src/routes/health.ts (1)
handleHealthRequest(3-12)src/routes/test.ts (1)
handleTestRequest(4-19)
tests/e2e/endpoints.test.ts (1)
src/server.ts (1)
fetch(11-34)
src/routes/screenshots.ts (2)
src/schemas/screenshot.schema.ts (1)
screenshotSchema(3-90)src/config/browser.ts (2)
defaultContext(4-9)browser(13-16)
src/routes/reports.ts (3)
src/schemas/lighthouse.schema.ts (1)
lighthouseSchema(3-73)src/config/browser.ts (2)
defaultContext(4-9)browser(13-16)src/config/lighthouse.ts (1)
lighthouseConfigs(4-7)
tests/unit/screenshot.schema.test.ts (1)
src/schemas/screenshot.schema.ts (1)
screenshotSchema(3-90)
src/routes/test.ts (2)
src/config/browser.ts (2)
browser(13-16)defaultContext(4-9)src/utils/test-page.ts (1)
generateTestHTML(1-326)
🔇 Additional comments (19)
.gitignore (1)
137-147: LGTM!The added ignore patterns appropriately exclude macOS artifacts (.DS_Store), IDE configurations (.idea/), and test-related artifacts (test-screenshots/, comparison/, .lighthouse/) that should not be tracked in version control.
tsconfig.json (1)
1-17: LGTM!The TypeScript configuration is well-structured for Bun runtime with appropriate compiler options. The strict mode, ES2022 target, and bundler module resolution align with modern best practices and Bun's requirements.
package.json (1)
8-17: Scripts are correctly configured for Bun.The npm scripts properly use Bun commands for running, testing, and building the application, aligning with the overall migration to Bun runtime.
.github/workflows/check.yml (1)
21-25: Install and check steps are properly configured.The workflow correctly uses
bun install --frozen-lockfilefor reproducible dependency installation and runs the ESLint check viabun run check..github/workflows/lint.yml (1)
21-25: Install and lint steps are properly configured.The workflow correctly uses
bun install --frozen-lockfileand runs the Biome lint check viabun run lint.Dockerfile (3)
1-8: Base image and dependency installation are correctly configured.The Dockerfile properly uses the Bun Alpine base image, copies the lockfile, installs production dependencies with frozen lockfile, and cleans up caches to minimize image size.
26-40: Security: Non-root user configuration is properly implemented.Creating a dedicated
chromeuser and switching to it (lines 26, 40) follows container security best practices by avoiding running as root. The ownership change (line 38) ensures the application has proper permissions.
42-43: Entrypoint and command are correctly configured.Using
tinias the entrypoint ensures proper signal handling and zombie process reaping, and the CMD correctly starts the Bun server.eslint.config.js (2)
1-15: ESLint configuration properly updated for TypeScript and Bun.The configuration correctly imports TypeScript ESLint, targets appropriate file patterns, and declares the
Bunglobal as readonly. This aligns well with the migration to Bun and TypeScript.
16-28: TypeScript rules are well-configured.The TypeScript ESLint recommended config is included, and the custom
no-unused-varsrule with underscore-prefix ignore patterns follows common best practices for intentionally unused parameters and variables.src/config/lighthouse.ts (1)
1-7: LGTM!Clean aggregation of Lighthouse configurations with proper immutability via
as const. This provides a centralized access point for both mobile and desktop audit configs.src/schemas/index.ts (1)
1-2: LGTM!Standard barrel export pattern for schema modules.
src/routes/index.ts (1)
1-4: LGTM!Standard barrel export pattern for route handlers.
src/config/index.ts (1)
1-2: LGTM!Clean barrel exports for browser and lighthouse configurations.
.github/workflows/test.yml (3)
1-25: LGTM!Workflow setup follows best practices with frozen lockfile for reproducible builds.
45-50: LGTM!Good use of
if: always()to ensure container cleanup regardless of test results.
30-43: No issues found — port configuration is consistent.The health check correctly uses port 3000, which matches the hardcoded PORT=3000 and port mapping (3000:3000) in docker-compose.yml. There is no port mismatch or flexibility concern since the docker-compose configuration does not parameterize PORT—it is hardcoded throughout. The original review comment's concern about potential port divergence does not apply to this codebase.
Likely an incorrect or invalid review comment.
tests/unit/screenshot.schema.test.ts (1)
1-159: LGTM!Comprehensive test coverage for the screenshot schema. The test suite covers default values, custom configurations, validation boundaries, and error cases effectively.
src/server.ts (1)
1-8: LGTM!Clean imports and setup for the Bun server.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/routes/reports.ts (1)
77-82: Return matching Content-Type for requested report format.The handler still hardcodes
application/json, so callers asking for HTML or CSV get the wrong MIME type back. This breaks clients parsing the response body.Apply this diff to derive the header from the requested format:
- const report = Array.isArray(results.report) - ? results.report.join("") - : results.report; - return new Response(report, { - headers: { "Content-Type": "application/json" }, - }); + const report = Array.isArray(results.report) + ? results.report.join("") + : results.report; + + const contentType = body.html + ? "text/html; charset=utf-8" + : body.csv + ? "text/csv; charset=utf-8" + : "application/json"; + + return new Response(report, { + headers: { "Content-Type": contentType }, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)package.json(1 hunks)src/config/index.ts(1 hunks)src/routes/reports.ts(3 hunks)src/routes/screenshots.ts(4 hunks)src/routes/test.ts(1 hunks)tests/e2e/endpoints.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- src/config/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/routes/test.ts (1)
src/utils/test-page.ts (1)
generateTestHTML(1-326)
src/routes/screenshots.ts (1)
src/config/browser.ts (1)
browser(13-16)
src/routes/reports.ts (1)
src/config/browser.ts (1)
browser(13-16)
tests/e2e/endpoints.test.ts (1)
src/server.ts (1)
fetch(11-34)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/e2e/endpoints.test.ts (3)
54-221: LGTM: Thorough screenshot endpoint coverage.Excellent test coverage including success scenarios, format variations, viewport/clipping options, and error handling. The buffer size comparisons (fullPage vs partial, clipped vs full) are a clever way to verify the features work correctly.
Optional enhancement: Consider validating specific error messages in the error test cases (lines 162-215) for better diagnostics. Currently, tests only verify that an
errorproperty exists.
224-239: Strengthen assertions for lighthouse report validation.The test only verifies that response data is truthy (line 238), which is insufficient for a complex lighthouse report. Consider validating the report structure, required fields, and data types to ensure the endpoint returns properly formatted lighthouse results.
Example validation to add:
const data = await response.json(); -expect(data).toBeTruthy(); +expect(data).toBeTruthy(); +expect(data).toHaveProperty("scores"); +expect(data).toHaveProperty("audits"); +// Add more specific validations based on expected report structure
241-252: Add response content validation for desktop viewport test.This test only verifies the status code without checking the response content or structure. Add assertions similar to the mobile viewport test to ensure the desktop report is properly generated.
tests/unit/screenshot.schema.test.ts (2)
109-117: Consider adding a negative test case for invalid permissions.While the valid permissions test is good, consider adding a test that verifies invalid permission values are rejected to ensure the enum validation works correctly.
Example:
test("should reject invalid permissions", () => { const input = { url: "https://example.com", permissions: ["geolocation", "invalid-permission"], }; const result = screenshotSchema.safeParse(input); expect(result.success).toBe(false); });
419-448: Consider adding tests for negative clip coordinates.While the tests cover zero width/height rejection, consider adding tests to verify that negative x or y coordinates are properly rejected according to the schema's
min(0)constraint.Example:
test("should reject clip with negative x coordinate", () => { const input = { url: "https://example.com", clip: { x: -1, y: 0, width: 100, height: 100 }, }; const result = screenshotSchema.safeParse(input); expect(result.success).toBe(false); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/endpoints.test.ts(1 hunks)tests/unit/lighthouse.schema.test.ts(1 hunks)tests/unit/screenshot.schema.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/lighthouse.schema.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/endpoints.test.ts (1)
src/server.ts (1)
fetch(11-34)
tests/unit/screenshot.schema.test.ts (1)
src/schemas/screenshot.schema.ts (1)
screenshotSchema(3-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit and E2E Tests
🔇 Additional comments (7)
tests/e2e/endpoints.test.ts (4)
1-4: LGTM: Clean test setup.The configurable BASE_URL via environment variable is a good practice for running tests against different environments.
5-52: LGTM: Comprehensive basic endpoint tests.Good coverage of both success cases and method restrictions. The timestamp validation using regex is appropriate.
254-273: LGTM: Good error handling coverage.Properly tests invalid viewport rejection and method restrictions.
275-285: LGTM: Appropriate 404 coverage.Tests verify that unknown routes are properly handled with 404 responses.
tests/unit/screenshot.schema.test.ts (3)
1-53: LGTM! Clean test structure and proper validation.The basic validation tests are well-structured with appropriate use of
safeParsefor validation checking and type guards when accessing parsed data.
55-107: LGTM! Comprehensive format and quality validation.The tests correctly validate supported formats and quality constraints matching the schema definition.
160-356: Excellent boundary testing coverage!The boundary tests are comprehensive and correctly validate all edge cases for viewport, quality, sleep, timeout, and device scale factor constraints. The tests verify both valid boundaries and just-outside-boundary rejections.
What does this PR do?
Tests support.
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Chores
Bug Fixes