test: add Playwright UI tests and expand Kotlin test coverage#17
Merged
test: add Playwright UI tests and expand Kotlin test coverage#17
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces the Gradle/Robot-based UI test setup with Playwright end-to-end tests, removes the legacy workflow and Gradle UI run configuration, exposes a testable HTML-script builder, and adds Playwright config, fixtures, and multiple E2E and unit tests for player behavior and escaping. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Job
participant WS as Dev Web Server
participant PW as Playwright Runner
participant Browser as Headless Chromium
CI->>WS: start dev server (`npm run dev`) on :5173
CI->>PW: run `npx playwright test`
PW->>Browser: launch Chromium, addInitScript to inject `window.jetplay`
Browser->>WS: request `/` (baseURL)
WS-->>Browser: serve app (index.html + JS)
Browser->>PW: execute test actions (clicks, keypresses, media asserts)
PW-->>CI: report results (upload `ui/playwright-report` artifact on failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Adds Playwright-based UI coverage for the bundled Svelte player UI and expands Kotlin-side unit tests around player HTML/config generation, while removing the previous IntelliJ UI-robot workflow.
Changes:
- Add Playwright test harness + new UI specs for player states, transitions, and audio/video controls.
- Integrate Playwright into UI package scripts and CI (new UI Tests job), and ignore Playwright artifacts.
- Expand Kotlin test coverage for accepted media extensions and JS escaping/config script generation.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/tests/video-player.spec.ts | Adds E2E coverage for video playback controls, keyboard shortcuts, and overlay visibility. |
| ui/tests/audio-player.spec.ts | Adds E2E coverage for audio playback controls, shortcuts, and seeking. |
| ui/tests/states.spec.ts | Validates UI rendering for ready/downloading/loading/error states and custom strings. |
| ui/tests/transitions.spec.ts | Verifies window bridge callbacks transition the UI between states/players. |
| ui/tests/fixtures.ts | Introduces a Playwright fixture to inject window.jetplay config before navigation. |
| ui/playwright.config.ts | Defines Playwright config, baseURL, and a dev-server-backed webServer. |
| ui/package.json | Adds Playwright scripts and dependency. |
| ui/package-lock.json | Locks Playwright dependency tree. |
| ui/.gitignore | Ignores Playwright report and test-results outputs. |
| src/test/kotlin/dev/twango/jetplay/editor/MediaFileEditorProviderTest.kt | Expands extension acceptance/rejection coverage. |
| src/test/kotlin/dev/twango/jetplay/browser/PlayerHtmlLoaderTest.kt | Adds unit tests for generated config script contents/escaping. |
| src/test/kotlin/dev/twango/jetplay/browser/PlayerBridgeEscapeTest.kt | Adds focused unit tests for JS escaping behavior. |
| src/main/kotlin/dev/twango/jetplay/browser/PlayerHtmlLoader.kt | Makes config-script builder testable (moved to companion). |
| build.gradle.kts | Removes IDE UI-test runner configuration tied to the old robot-based approach. |
| .github/workflows/run-ui-tests.yml | Removes the legacy IntelliJ UI-robot workflow. |
| .github/workflows/build.yml | Adds a CI job to run Playwright UI tests and upload the report on failure. |
Files not reviewed (1)
- ui/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ui/playwright.config.ts (1)
3-16: Configuration looks good with one consideration.The Playwright setup is well-structured with appropriate CI-specific retries and web server configuration.
Consider adding Firefox and/or WebKit to the test matrix in the future to catch browser-specific rendering issues, especially since this is a media player UI:
💡 Optional: Multi-browser configuration
use: { baseURL: 'http://localhost:5173', - browserName: 'chromium', }, +projects: [ + { name: 'chromium', use: { browserName: 'chromium' } }, + { name: 'firefox', use: { browserName: 'firefox' } }, +],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/playwright.config.ts` around lines 3 - 16, The Playwright config currently pins browserName to 'chromium' in the defineConfig block (use.browserName) which limits cross-browser coverage; update the config to run tests across multiple browsers by replacing the single-use browserName setting with a projects matrix (or multiple defineConfig projects) that includes 'chromium', 'firefox', and optionally 'webkit', while keeping existing settings (testDir, timeout, retries, webServer) intact and reusing reuseExistingServer behavior for CI; locate the defineConfig export and modify the use/browserName or add projects entries to introduce the multi-browser configuration..github/workflows/build.yml (1)
88-90: Consider removing thebuilddependency for faster CI.The
ui-testjob depends onbuildbut doesn't consume any artifacts from it. The UI tests run against the dev server (npm run dev), not the built plugin. Removing this dependency would allowui-testto run in parallel with the build, reducing overall CI time.♻️ Suggested change
ui-test: name: UI Tests - needs: [ build ] runs-on: ubuntu-latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 88 - 90, The ui-test job currently lists a dependency on the build job which is unnecessary; update the GitHub Actions workflow by removing "build" from the needs array of the ui-test job (job name: ui-test) so it no longer depends on the build job and can run in parallel—ensure you only modify the needs: [ build ] entry for ui-test and leave other job relationships unchanged.ui/tests/fixtures.ts (1)
3-18: Avoid duplicating thejetplayconfig contract in tests.
JetplayConfighere mirrorsui/src/global.d.tsand can drift over time. Prefer reusing a single source of truth (for example, a shared exported type used by bothglobal.d.tsand tests).♻️ Suggested direction
-interface JetplayConfig { - mediaUrl?: string - fileName?: string - fileExtension?: string - isVideo?: boolean - state?: 'downloading' | 'loading' | 'ready' | 'error' - errorMessage?: string - transcodingReason?: string - downloadingReason?: string - ui?: { - downloadingLabel?: string - transcodingLabel?: string - transcodingTip?: string - errorTitle?: string - } -} +type JetplayConfig = NonNullable<Window['jetplay']>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/tests/fixtures.ts` around lines 3 - 18, The test file defines a duplicate JetplayConfig interface which can drift from the canonical type; replace this local JetplayConfig with the shared exported type from the app (e.g., the type declared in ui/src/global.d.ts), import or reference that single exported symbol in the tests instead of redefining it, and remove the local interface so tests consume the authoritative contract (update any test imports/fixtures to use the shared type name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/tests/audio-player.spec.ts`:
- Around line 26-58: The tests are flaky because they interact with the audio
element before metadata is loaded; update both tests to wait for media readiness
by awaiting the audio element's loadedmetadata (or readyState >= HAVE_METADATA)
before performing seeks or reads: in the "skip forward" test, after starting
playback (the play button click) await the audio's metadata ready event before
capturing timeBefore and clicking skipForwardBtn; in the "skip backward" test,
after setting el.currentTime = 15 await the audio's metadata ready event (or
verify readyState) before clicking skipBackBtn and asserting timeAfter; use the
existing audio locator and the skipForwardBtn/skipBackBtn variables to place the
waits so seeks are effective.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 88-90: The ui-test job currently lists a dependency on the build
job which is unnecessary; update the GitHub Actions workflow by removing "build"
from the needs array of the ui-test job (job name: ui-test) so it no longer
depends on the build job and can run in parallel—ensure you only modify the
needs: [ build ] entry for ui-test and leave other job relationships unchanged.
In `@ui/playwright.config.ts`:
- Around line 3-16: The Playwright config currently pins browserName to
'chromium' in the defineConfig block (use.browserName) which limits
cross-browser coverage; update the config to run tests across multiple browsers
by replacing the single-use browserName setting with a projects matrix (or
multiple defineConfig projects) that includes 'chromium', 'firefox', and
optionally 'webkit', while keeping existing settings (testDir, timeout, retries,
webServer) intact and reusing reuseExistingServer behavior for CI; locate the
defineConfig export and modify the use/browserName or add projects entries to
introduce the multi-browser configuration.
In `@ui/tests/fixtures.ts`:
- Around line 3-18: The test file defines a duplicate JetplayConfig interface
which can drift from the canonical type; replace this local JetplayConfig with
the shared exported type from the app (e.g., the type declared in
ui/src/global.d.ts), import or reference that single exported symbol in the
tests instead of redefining it, and remove the local interface so tests consume
the authoritative contract (update any test imports/fixtures to use the shared
type name).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e61ab866-3c30-4f85-a0ab-0f520378317c
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.github/workflows/build.yml.github/workflows/run-ui-tests.ymlbuild.gradle.ktssrc/main/kotlin/dev/twango/jetplay/browser/PlayerHtmlLoader.ktsrc/test/kotlin/dev/twango/jetplay/browser/PlayerBridgeEscapeTest.ktsrc/test/kotlin/dev/twango/jetplay/browser/PlayerHtmlLoaderTest.ktsrc/test/kotlin/dev/twango/jetplay/editor/MediaFileEditorProviderTest.ktui/.gitignoreui/package.jsonui/playwright.config.tsui/public/assetsui/tests/audio-player.spec.tsui/tests/fixtures.tsui/tests/states.spec.tsui/tests/transitions.spec.tsui/tests/video-player.spec.ts
💤 Files with no reviewable changes (2)
- .github/workflows/run-ui-tests.yml
- build.gradle.kts
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Contributor
Qodana Community for JVMIt seems all right 👌 No new problems were found according to the checks applied 💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2025.1.1
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- ui/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ontrol visibility
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Tests
Chores