feature(trace-viewer): embedded mode support PoC #483
Conversation
|
I'm supportive for those, let me know when they are no longer drafts! |
|
It now supports toggle between dark / light theme: theme_toggle.mp4 |
ruifigueira
force-pushed
the
feat/embedded-trace-viewer
branch
from
090eaeb to
9cc5f7e
Compare
ruifigueira
force-pushed
the
feat/embedded-trace-viewer
branch
from
009bbe1 to
9007b22
Compare
|
I added a few tests. They require a playwright version with changes from microsoft/playwright#30885 , and that's why I left them as wip for now. |
|
It's now working with vscode SSH remote dev and dev containers. One nice thing is that there's no need to expose playwright server as |
ruifigueira
force-pushed
the
feat/embedded-trace-viewer
branch
2 times, most recently
from
1bb183d to
1f55027
Compare
|
It also works in codespaces: codespaces.mp4 |
ruifigueira
force-pushed
the
feat/embedded-trace-viewer
branch
from
1f55027 to
9423cd2
Compare
ruifigueira
force-pushed
the
feat/embedded-trace-viewer
branch
from
4dc39ac to
e782ce5
Compare
ruifigueira
force-pushed
the
feat/embedded-trace-viewer
branch
from
2c1d2e7 to
cfc7901
Compare
ruifigueira
force-pushed
the
feat/embedded-trace-viewer
branch
from
6a87213 to
b5f3b90
Compare
There was a problem hiding this comment.
The change looks good. Please split it into the chunks so that I could do thorough review. These look like good chunks, but open to variations:
- WebView support (nonce)
- Selection strategy (selected item)
- Trace viewer abstraction + ShallowTraceViewer
- Additional embedded trace viewer
| @@ -70,6 +70,11 @@ | |||
| "command": "pw.extension.toggle.showTrace", | |||
| "title": "%contributes.command.pw.extension.toggle.showTrace%" | |||
| }, | |||
| { | |||
| "category": "Test", | |||
| "command": "pw.extension.toggle.embedTraceViewer", | |||
There was a problem hiding this comment.
| "command": "pw.extension.toggle.embedTraceViewer", | |
| "command": "pw.extension.toggle.embeddedTraceViewer", |
| @@ -208,6 +207,12 @@ export class Extension implements RunHooks { | |||
| vscode.commands.registerCommand('pw.extension.command.clearCache', async () => { | |||
| await this._models.selectedModel()?.clearCache(); | |||
| }), | |||
| vscode.commands.registerCommand('pw.extension.command.openTrace', async (uri?: vscodeTypes.Uri) => { | |||
There was a problem hiding this comment.
Let's add this separately.
| this._traceViewers = undefined; | ||
| } | ||
|
|
||
| async availableTraceViewers(): Promise<TraceViewer[]> { |
There was a problem hiding this comment.
Why not traceViewer() that would return enabled instance? You don't seem to save much via having the enabled check on the call site. Also, if those are shallow objects, why not created them in the constructor? It would read better.
There was a problem hiding this comment.
I was doing it that way because I was only checking if the trace viewer supported the playwright version in the extension, so I needed to send both implementations.
I'm now changing that to ensure that we only request the enabled trace viewer when needed, and so checking the playwright version can be done here or in testModel, so that we can only pass the right trace viewer to the call site.
|
|
||
| export type TraceViewer = SpawnTraceViewer | EmbeddedTraceViewer; | ||
|
|
||
| export class SpawnTraceViewer extends DisposableBase { |
There was a problem hiding this comment.
Let's put these into separate files. When splitting the change, we can land SpawnTraceViewer in one PR and follow up with adding EmbeddedTraceViewer. That way we would see if there is something funny with the chosen modularity.
PoC of trace-viewer embedded in VSCode.
It allows to open trace zip files as well as live traces of running tests.
embedded_trace_viewer.mp4
Depends on microsoft/playwright#30885