chore: migrate Trace Viewer tests to use real Trace viewer #1830
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates Trace Viewer tests from a JSON-based approach to using the actual Trace Viewer web application for validation. The change improves test coverage by testing the real UI components instead of just parsing trace files.
- Replaced JSON parsing and verification of trace events with actual Trace Viewer UI interactions
- Added a new
TraceViewerPagehelper class to encapsulate Trace Viewer UI operations - Enhanced the test server to serve static files from the filesystem to support hosting the Trace Viewer
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| TestTracing.java | Migrated all trace verification logic from JSON parsing to Trace Viewer UI testing, added TraceViewerPage helper class |
| Server.java | Added static file serving capability to support hosting Trace Viewer from filesystem |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
playwright/src/test/java/com/microsoft/playwright/TestTracing.java
Outdated
Show resolved
Hide resolved
| @@ -187,7 +193,19 @@ public void handle(HttpExchange exchange) throws IOException { | |||
| path = "/index.html"; | |||
| } | |||
|
|
|||
| // Resources from "src/test/resources/" are copied to "resources/" directory in the jar. | |||
| // If static files directory is set, serve from filesystem first | |||
| if (staticFilesDirectory != null) { | |||
There was a problem hiding this comment.
underneath we do something similar but via getResourceAsStream which got introduced here - do you remember why?
There was a problem hiding this comment.
This is the standard way to read resources in a maven package (they move from the source into the test package artifact). I'd keep using it with the custom base dir too, can we do that?
mxschmitt
force-pushed
the
trace-viewer
branch
from
d28e6f8 to
409fa48
Compare
| @@ -187,7 +193,19 @@ public void handle(HttpExchange exchange) throws IOException { | |||
| path = "/index.html"; | |||
| } | |||
|
|
|||
| // Resources from "src/test/resources/" are copied to "resources/" directory in the jar. | |||
| // If static files directory is set, serve from filesystem first | |||
| if (staticFilesDirectory != null) { | |||
There was a problem hiding this comment.
This is the standard way to read resources in a maven package (they move from the source into the test package artifact). I'd keep using it with the custom base dir too, can we do that?
| // If static files directory is set, serve from filesystem first | ||
| if (staticFilesDirectory != null) { | ||
| java.nio.file.Path filePath = staticFilesDirectory.resolve(path.substring(1)); // Remove leading / | ||
| if (java.nio.file.Files.exists(filePath) && !java.nio.file.Files.isDirectory(filePath)) { |
| } | ||
|
|
||
| private void showTraceViewer(Path tracePath, TraceViewerConsumer callback) throws Exception { | ||
| Path driverDir = Driver.ensureDriverInstalled(java.util.Collections.emptyMap(), true).driverDir(); |
There was a problem hiding this comment.
Move this method along with the trace viewer POM into its own file? Also consider turning the viewer into a fixture.
Fixes #1829