Add e2e tests with Vitest browser mode and Playwright#6
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces end-to-end testing infrastructure for the tinysaver library using Vitest with Playwright browser support. Changes include a new e2e test configuration file, a comprehensive browser-based test suite covering save operations and browser detection, required dev dependencies, build script updates, and documentation. Changes
Sequence DiagramsequenceDiagram
participant Vitest as Vitest<br/>(Test Runner)
participant Browser as Browser Instance<br/>(Playwright)
participant Library as tinysaver<br/>Library
participant APIs as Browser APIs<br/>(DOM, Canvas)
Vitest->>Browser: Launch browser instance
Browser->>Browser: Initialize test environment
Vitest->>Library: Execute test case
Library->>APIs: Detect browser capabilities
APIs-->>Library: Return capabilities
Library-->>Vitest: Assert detection results
Vitest->>Library: Execute save operation
Library->>APIs: Trigger download/save
APIs-->>Library: Confirm operation
Library->>Library: Invoke lifecycle callbacks
Library-->>Vitest: Assert save completed
Vitest->>Browser: Run across multiple browsers
Browser-->>Vitest: Report results
Vitest->>Vitest: Validate cross-browser compatibility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
- Install @vitest/browser, @vitest/browser-playwright, and playwright - Create vitest.config.e2e.ts for multi-browser e2e testing (chromium, firefox, webkit) - Add comprehensive e2e test suite covering: * Browser detection functionality * saveAs with Blob objects * saveText with various content types * saveJSON with complex objects * saveCanvas with different formats * Error handling scenarios * Lifecycle callbacks * Cross-browser compatibility tests - Update package.json with test:e2e script - Update vitest.config.ts to exclude e2e tests from unit tests - Add Playwright artifacts to .gitignore - Create detailed README for e2e tests with setup and CI/CD instructions Co-authored-by: ntnyq <22659150+ntnyq@users.noreply.github.com>
- Fix BlobCallback type to use proper HTMLCanvasElement.toBlob signature - Improve multiline string readability using escape sequences - Extract E2E_TEST_TIMEOUT constant for better maintainability Co-authored-by: ntnyq <22659150+ntnyq@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/e2e/tinysaver.test.ts (3)
72-74: Consider removing the no-op assertion.
expect(true).toBe(true)is a trivial assertion that always passes. If the intent is to verifyonStartwas called, track it with a boolean flag similar to other tests in this file, or simply remove this assertion since reaching the callback already proves it was invoked.♻️ Suggested improvement
onStart: () => { - expect(true).toBe(true) + // onStart callback reached - no additional assertion needed },Or track with a flag like the other tests:
+ let startCalled = false return new Promise<void>(resolve => { saveAs(blob, 'test-with-options.txt', { disableClick: true, autoBom: true, onStart: () => { - expect(true).toBe(true) + startCalled = true }, onComplete: () => { completed = true + expect(startCalled).toBe(true) expect(completed).toBe(true) resolve() },
382-390: Weak assertion that always passes.
expect(hasDownload || hasMsSave).toBeDefined()will always pass because booleans are always defined. Based on the comment stating these should be "mutually exclusive" and modern browsers should have download support, consider a more meaningful assertion:♻️ Suggested fix
// They should be mutually exclusive in most cases // Modern browsers have download support, IE has msSaveBlob - expect(hasDownload || hasMsSave).toBeDefined() + // At least one mechanism should be available + expect(hasDownload || hasMsSave).toBe(true)
416-434: Test name is misleading: these are concurrent, not sequential saves.The test initiates all three saves in a loop before calling
Promise.all, making them concurrent rather than sequential. Consider renaming for clarity:♻️ Suggested fix
- test('should handle multiple sequential saves', async () => { + test('should handle multiple concurrent saves', async () => {If truly sequential saves are intended, await each save before starting the next:
test('should handle multiple sequential saves', async () => { for (let i = 0; i < 3; i++) { await new Promise<void>(resolve => { const blob = new Blob([`content ${i}`], { type: 'text/plain' }) saveAs(blob, `file-${i}.txt`, { disableClick: true, onComplete: () => resolve(), }) }) } })tests/e2e/README.md (2)
19-28: Align Playwright installation command with CI example.The installation command at line 24 differs from the CI example at line 81. The CI uses
npx playwright install --with-deps, which installs both browsers and system dependencies in one step, matching the PR objectives. Consider updating the manual installation instructions to use the same command for consistency and simplicity.📝 Proposed update for consistency
# Install dependencies pnpm install # Install Playwright browsers -npx playwright install chromium firefox webkit - -# Install system dependencies for Playwright (Linux only) -npx playwright install-deps +npx playwright install --with-deps
30-38: Clarify or remove redundant command.Lines 36-37 suggest running tests in a specific browser, but the command is identical to line 34. This doesn't provide actionable guidance for running a single browser. Consider either removing the redundant entry or showing how to actually run a specific browser (e.g., via environment variable, CLI flag, or temporary config modification).
💡 Proposed improvement
Option 1: Remove the redundant command:
```bash # Run e2e tests (all browsers configured in vitest.config.e2e.ts) pnpm test:e2e - -# Run e2e tests in specific browser (edit vitest.config.e2e.ts to select) -pnpm test:e2eOption 2: Show actual single-browser execution (if supported): ```diff ```bash # Run e2e tests (all browsers configured in vitest.config.e2e.ts) pnpm test:e2e -# Run e2e tests in specific browser (edit vitest.config.e2e.ts to select) -pnpm test:e2e +# Run e2e tests with a specific browser filter +pnpm test:e2e --project=chromium</details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6fb949fa857d3fbc6611ad4b997d82e484c31bf4 and cb1b3a0342055ac3039b3f5eec4ca212c87d619e. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (6)</summary> * `.gitignore` * `package.json` * `tests/e2e/README.md` * `tests/e2e/tinysaver.test.ts` * `vitest.config.e2e.ts` * `vitest.config.ts` </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (1)</summary> <details> <summary>tests/e2e/tinysaver.test.ts (2)</summary><blockquote> <details> <summary>src/BrowserDetector.ts (1)</summary> * `BrowserDetector` (4-67) </details> <details> <summary>src/core.ts (1)</summary> * `saveAs` (13-19) </details> </blockquote></details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>tests/e2e/README.md</summary> [grammar] ~111-~111: Ensure spelling is correct Context: ...eeded ``` ### Timeout Issues If tests timeout, increase the timeout in `vitest.config... (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) </details> </details> </details> <details> <summary>🔇 Additional comments (10)</summary><blockquote> <details> <summary>.gitignore (1)</summary><blockquote> `13-17`: **LGTM!** The Playwright-related ignore patterns are correctly added. These cover the standard artifacts generated by Playwright test runs (test results, reports, and cache directories). </blockquote></details> <details> <summary>vitest.config.ts (1)</summary><blockquote> `6-6`: **LGTM!** The exclude pattern correctly separates E2E tests from unit test runs. This ensures `pnpm test` runs only unit tests while `pnpm test:e2e` handles browser-based tests via the dedicated config. </blockquote></details> <details> <summary>package.json (2)</summary><blockquote> `43-46`: **LGTM!** Good addition of `test:e2e` to the release check workflow. This ensures E2E tests pass before any release, catching browser-specific regressions early. --- `54-55`: **Dependency versions are compatible.** Playwright ^1.57.0 is compatible with @vitest/browser-playwright ^4.0.17 (peer dependency accepts any Playwright version), and vitest ^4.0.17 satisfies the required peer dependency. The consistent versioning across Vitest packages is correct. </blockquote></details> <details> <summary>vitest.config.e2e.ts (1)</summary><blockquote> `1-21`: **LGTM!** Well-structured E2E configuration. The extracted `E2E_TEST_TIMEOUT` constant improves maintainability, and testing across Chromium, Firefox, and WebKit provides comprehensive cross-browser coverage. Headless mode is appropriate for CI environments. </blockquote></details> <details> <summary>tests/e2e/tinysaver.test.ts (5)</summary><blockquote> `1-38`: **LGTM!** Good baseline tests for `BrowserDetector`. The assertions correctly expect modern browser capabilities (download support present, msSaveBlob absent) which aligns with the Chromium/Firefox/WebKit test matrix. --- `101-148`: **LGTM!** Good coverage of `saveText` functionality including multi-line content and custom MIME types. --- `207-225`: **Note: `undefined` is omitted in JSON output.** This test verifies the save completes successfully, but note that `JSON.stringify` omits keys with `undefined` values. The resulting JSON will only contain `{"nullValue":null,"number":42}`. If the intent is to test edge cases, consider adding a comment clarifying this expected behavior. --- `228-312`: **LGTM!** Good coverage of canvas save functionality across PNG, JPEG, and WebP formats. The shared canvas setup in `beforeAll` is efficient. --- `314-348`: **Good error handling test structure.** The dual-path approach (catching via `onError` callback or thrown exception) correctly handles both error propagation methods, and the error message expectation matches the implementation. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive end-to-end testing infrastructure using Vitest browser mode with Playwright, enabling real browser testing across Chromium, Firefox, and WebKit.
Changes:
- Added E2E test configuration with multi-browser support via
vitest.config.e2e.ts - Implemented 22 E2E tests covering browser detection, file operations, error handling, and lifecycle callbacks
- Updated dependencies to include Vitest 4.0.17, Playwright, and browser testing packages
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Excludes e2e tests from unit test runs |
| vitest.config.e2e.ts | Configures browser mode with Playwright for Chromium, Firefox, and WebKit |
| tests/e2e/tinysaver.test.ts | Implements comprehensive E2E tests for all library functions |
| tests/e2e/README.md | Documents setup, usage, and CI/CD integration |
| package.json | Adds test:e2e script and updates dependencies |
| .gitignore | Adds Playwright artifacts to ignore list |
| pnpm-lock.yaml | Updates lockfile with new dependencies |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/e2e/README.md
Outdated
| # Run e2e tests (all browsers configured in vitest.config.e2e.ts) | ||
| pnpm test:e2e | ||
|
|
||
| # Run e2e tests in specific browser (edit vitest.config.e2e.ts to select) | ||
| pnpm test:e2e |
There was a problem hiding this comment.
The documentation shows the same command twice for running e2e tests. The second command comment suggests running tests in a specific browser by editing the config file, but both commands are identical. Consider clarifying the second command or removing it if there's no specific browser selection command available.
| # Run e2e tests (all browsers configured in vitest.config.e2e.ts) | |
| pnpm test:e2e | |
| # Run e2e tests in specific browser (edit vitest.config.e2e.ts to select) | |
| pnpm test:e2e | |
| # Run e2e tests (browsers configured in vitest.config.e2e.ts) | |
| pnpm test:e2e | |
| # To run tests in a specific browser, update vitest.config.e2e.ts (e.g. select only chromium) | |
| # and rerun the command above. |
There was a problem hiding this comment.
Fixed in f14b179. Updated the documentation to clarify that browser selection requires editing the config file, and removed the duplicate command.
tests/e2e/tinysaver.test.ts
Outdated
| disableClick: true, | ||
| autoBom: true, | ||
| onStart: () => { | ||
| expect(true).toBe(true) |
There was a problem hiding this comment.
The assertion 'expect(true).toBe(true)' is a no-op that doesn't test any meaningful behavior. Consider either testing some actual condition within the onStart callback (e.g., checking that the blob is defined) or removing this assertion if no verification is needed at this point.
| expect(true).toBe(true) | |
| expect(blob).toBeInstanceOf(Blob) |
There was a problem hiding this comment.
Fixed in f14b179. Replaced the no-op assertion with expect(blob).toBeInstanceOf(Blob) to verify the blob is properly defined.
| test('should save canvas as WebP', async () => { | ||
| let completed = false | ||
|
|
||
| return new Promise<void>(resolve => { | ||
| saveCanvas(canvas, 'test-canvas.webp', { | ||
| disableClick: true, | ||
| type: 'image/webp', | ||
| quality: 0.85, | ||
| onComplete: () => { | ||
| completed = true | ||
| expect(completed).toBe(true) | ||
| resolve() | ||
| }, | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
WebP format may not be supported in all browsers being tested. Firefox and older versions of some browsers may not support WebP image encoding via canvas.toBlob. Consider adding browser capability detection or making this test conditional based on browser support to avoid false failures.
There was a problem hiding this comment.
Fixed in f14b179. Added browser capability detection for WebP support. The test now checks if WebP is supported and skips gracefully if not, preventing false failures in browsers without WebP encoding support.
|
@copilot fix typecheck error |
tests/e2e/tinysaver.test.ts
Outdated
|
|
||
| expect(errorReceived).toBe(true) | ||
| expect(errorInstance).toBeInstanceOf(Error) | ||
| expect(errorInstance?.message).toContain('Failed to convert canvas to blob') |
There was a problem hiding this comment.
The typecheck error at line 346 was already fixed in commit f14b179 by using a non-null assertion operator (errorInstance!.message). Typecheck now passes successfully.
tests/e2e/README.md
Outdated
|
|
||
| ### Prerequisites | ||
|
|
||
| - Node.js 16+ |
There was a problem hiding this comment.
Updated in f88345a. Changed Node.js version requirement from 16+ to 20+ in the README prerequisites.
- Fix typecheck error with non-null assertion for errorInstance - Replace no-op assertion with meaningful blob instance check - Add WebP browser support detection to prevent false failures - Clarify README documentation for running specific browser tests Co-authored-by: ntnyq <22659150+ntnyq@users.noreply.github.com>
- Update README prerequisites to require Node.js 20+ instead of 16+ - Aligns with project's target Node.js version Co-authored-by: ntnyq <22659150+ntnyq@users.noreply.github.com>
tests/e2e/tinysaver.test.ts
Outdated
|
|
||
| // They should be mutually exclusive in most cases | ||
| // Modern browsers have download support, IE has msSaveBlob | ||
| expect(hasDownload || hasMsSave).toBeDefined() |
There was a problem hiding this comment.
Fixed in ad6ed8f. Changed from .toBeDefined() to .toBe(true) to properly verify that at least one download method is available.
tests/e2e/tinysaver.test.ts
Outdated
| }) | ||
| }) | ||
|
|
||
| test('should handle multiple sequential saves', async () => { |
There was a problem hiding this comment.
Fixed in ad6ed8f. Renamed the test from "sequential" to "parallel" to accurately reflect that it uses Promise.all() which runs saves in parallel, not sequence.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/e2e/tinysaver.test.ts
Outdated
|
|
||
| // They should be mutually exclusive in most cases | ||
| // Modern browsers have download support, IE has msSaveBlob | ||
| expect(hasDownload || hasMsSave).toBeDefined() |
There was a problem hiding this comment.
The assertion expect(hasDownload || hasMsSave).toBeDefined() will always pass since the result of a boolean OR operation is always defined (either true or false). Consider changing this to expect(hasDownload || hasMsSave).toBe(true) to verify that at least one download method is available, or remove this assertion if it's meant to just check that the values are defined.
| expect(hasDownload || hasMsSave).toBeDefined() | |
| expect(hasDownload || hasMsSave).toBe(true) |
There was a problem hiding this comment.
Fixed in ad6ed8f. Changed the assertion to .toBe(true) as suggested.
package.json
Outdated
| "prepublishOnly": "pnpm run build", | ||
| "release": "run-s release:check release:version", | ||
| "release:check": "run-s lint typecheck test", | ||
| "release:check": "run-s lint typecheck test test:e2e", |
There was a problem hiding this comment.
Adding test:e2e to the release:check script means developers must have Playwright browsers installed locally to run releases. Since the PR description indicates that running e2e tests in CI is not yet implemented, consider making the e2e tests optional in the release check until CI support is added, or ensure all contributors are aware they need to run npx playwright install before releasing. Alternatively, you could create a separate release script that skips e2e tests for local releases.
| "release:check": "run-s lint typecheck test test:e2e", | |
| "release:check": "run-s lint typecheck test", | |
| "release:check:ci": "run-s lint typecheck test test:e2e", |
There was a problem hiding this comment.
Fixed in ad6ed8f. Removed e2e tests from release:check and added release:check:full for CI use. This allows local releases without requiring Playwright browsers to be installed.
|
@copilot resolve reviews above |
- Fix assertion to check if at least one download method is available (line 399) - Rename test from 'sequential' to 'parallel' to match actual behavior - Make e2e tests optional in release:check (add release:check:full for CI) - This allows local releases without requiring Playwright browsers installed Co-authored-by: ntnyq <22659150+ntnyq@users.noreply.github.com>
|
@copilot Add e2e test in workflows ci and only run it if all tests matrix are successful. |
- Add separate e2e job that runs after check job succeeds - Install Playwright browsers with dependencies in CI - E2E tests now run in CI after all main checks pass Co-authored-by: ntnyq <22659150+ntnyq@users.noreply.github.com>
Added in b10ad99. Created a separate
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
E2E Tests Implementation Plan
Implementation Complete: E2E test infrastructure is fully set up with all review feedback addressed. Tests include browser capability detection for WebP format, proper TypeScript type assertions, documentation updated for Node.js 20+, e2e tests are optional for local releases (use
release:check:fullto include them), and e2e tests now run automatically in CI after all main checks pass successfully.CI Integration
The e2e tests are configured to run in GitHub Actions CI workflow:
e2ejob that depends on thecheckjobOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.