feat(inspector): render errors (#5459)

pavelfeldman · web-flow · commit 8b9a2afd3d69 · 2021-02-13T22:13:51.000-08:00
diff --git a/index.js b/index.js
@@ -14,5 +14,4 @@
  * limitations under the License.
  */
 
-const { setUnderTest } = require('./lib/utils/utils');
 module.exports = require('./lib/inprocess');
diff --git a/src/dispatchers/dispatcher.ts b/src/dispatchers/dispatcher.ts
@@ -214,6 +214,7 @@ export class DispatcherConnection {
       this.onmessage({ id, result: this._replaceDispatchersWithGuids(result) });
     } catch (e) {
       // Dispatching error
+      callMetadata.error = e.message;
       if (callMetadata.log.length)
         rewriteErrorMessage(e, e.message + formatLogRecording(callMetadata.log) + kLoggingNote);
       this.onmessage({ id, error: serializeError(e) });
diff --git a/src/server/browserContext.ts b/src/server/browserContext.ts
@@ -345,8 +345,8 @@ export abstract class BrowserContext extends SdkObject {
     }
   }
 
-  async extendInjectedScript(source: string) {
-    const installInFrame = (frame: frames.Frame) => frame.extendInjectedScript(source).catch(e => {});
+  async extendInjectedScript(source: string, arg?: any) {
+    const installInFrame = (frame: frames.Frame) => frame.extendInjectedScript(source, arg).catch(() => {});
     const installInPage = (page: Page) => {
       page.on(Page.Events.InternalFrameNavigatedToNewDocument, installInFrame);
       return Promise.all(page.frames().map(installInFrame));
diff --git a/src/server/instrumentation.ts b/src/server/instrumentation.ts
@@ -39,7 +39,7 @@ export type CallMetadata = {
   params: any;
   stack?: StackFrame[];
   log: string[];
-  error?: Error;
+  error?: string;
   point?: Point;
 };
 
diff --git a/src/server/supplements/injected/recorder.ts b/src/server/supplements/injected/recorder.ts
@@ -51,8 +51,10 @@ export class Recorder {
   private _actionPointElement: HTMLElement;
   private _actionPoint: Point | undefined;
   private _actionSelector: string | undefined;
+  private _params: { isUnderTest: boolean; };
 
-  constructor(injectedScript: InjectedScript) {
+  constructor(injectedScript: InjectedScript, params: { isUnderTest: boolean }) {
+    this._params = params;
     this._injectedScript = injectedScript;
     this._outerGlassPaneElement = html`
       <x-pw-glass style="
@@ -76,7 +78,7 @@ export class Recorder {
       </x-pw-glass-inner>`;
 
     // Use a closed shadow root to prevent selectors matching our internal previews.
-    this._glassPaneShadow = this._outerGlassPaneElement.attachShadow({ mode: 'closed' });
+    this._glassPaneShadow = this._outerGlassPaneElement.attachShadow({ mode: this._params.isUnderTest ? 'open' : 'closed' });
     this._glassPaneShadow.appendChild(this._innerGlassPaneElement);
     this._glassPaneShadow.appendChild(this._actionPointElement);
     this._glassPaneShadow.appendChild(html`
diff --git a/src/server/supplements/recorder/recorderApp.ts b/src/server/supplements/recorder/recorderApp.ts
@@ -26,6 +26,7 @@ import { internalCallMetadata } from '../../instrumentation';
 import type { CallLog, EventData, Mode, Source } from './recorderTypes';
 import { BrowserContext } from '../../browserContext';
 import { isUnderTest } from '../../../utils/utils';
+import { RecentLogsCollector } from '../../../utils/debugLogger';
 
 const readFileAsync = util.promisify(fs.readFile);
 
@@ -41,11 +42,13 @@ declare global {
 
 export class RecorderApp extends EventEmitter {
   private _page: Page;
+  readonly wsEndpoint: string | undefined;
 
-  constructor(page: Page) {
+  constructor(page: Page, wsEndpoint: string | undefined) {
     super();
     this.setMaxListeners(0);
     this._page = page;
+    this.wsEndpoint = wsEndpoint;
   }
 
   async close() {
@@ -90,28 +93,45 @@ export class RecorderApp extends EventEmitter {
 
   static async open(inspectedContext: BrowserContext): Promise<RecorderApp> {
     const recorderPlaywright = createPlaywright(true);
+    const args = [
+      '--app=data:text/html,',
+      '--window-size=600,600',
+      '--window-position=1280,10',
+    ];
+    if (isUnderTest())
+      args.push(`--remote-debugging-port=0`);
     const context = await recorderPlaywright.chromium.launchPersistentContext(internalCallMetadata(), '', {
       sdkLanguage: inspectedContext._options.sdkLanguage,
-      args: [
-        '--app=data:text/html,',
-        '--window-size=600,600',
-        '--window-position=1280,10',
-      ],
+      args,
       noDefaultViewport: true,
       headless: isUnderTest() && !inspectedContext._browser.options.headful
     });
-
+    const wsEndpoint = isUnderTest() ? await this._parseWsEndpoint(context._browser.options.browserLogsCollector) : undefined;
     const controller = new ProgressController(internalCallMetadata(), context._browser);
     await controller.run(async progress => {
       await context._browser._defaultContext!._loadDefaultContextAsIs(progress);
     });
 
     const [page] = context.pages();
-    const result = new RecorderApp(page);
+    const result = new RecorderApp(page, wsEndpoint);
     await result._init();
     return result;
   }
 
+  private static async _parseWsEndpoint(recentLogs: RecentLogsCollector): Promise<string> {
+    let callback: ((log: string) => void) | undefined;
+    const result = new Promise<string>(f => callback = f);
+    const check = (log: string) => {
+      const match = log.match(/DevTools listening on (.*)/);
+      if (match)
+        callback!(match[1]);
+    };
+    for (const log of recentLogs.recentLogs())
+      check(log);
+    recentLogs.on('log', check);
+    return result;
+  }
+
   async setMode(mode: 'none' | 'recording' | 'inspecting'): Promise<void> {
     await this._page.mainFrame()._evaluateExpression(((mode: Mode) => {
       window.playwrightSetMode(mode);
diff --git a/src/server/supplements/recorder/recorderTypes.ts b/src/server/supplements/recorder/recorderTypes.ts
@@ -34,11 +34,12 @@ export type CallLog = {
   title: string;
   messages: string[];
   status: 'in-progress' | 'done' | 'error' | 'paused';
+  error?: string;
 };
 
 export type SourceHighlight = {
   line: number;
-  type: 'running' | 'paused';
+  type: 'running' | 'paused' | 'error';
 };
 
 export type Source = {
diff --git a/src/server/supplements/recorderSupplement.ts b/src/server/supplements/recorderSupplement.ts
@@ -33,6 +33,7 @@ import { RecorderApp } from './recorder/recorderApp';
 import { CallMetadata, internalCallMetadata, SdkObject } from '../instrumentation';
 import { Point } from '../../common/types';
 import { CallLog, EventData, Mode, Source, UIState } from './recorder/recorderTypes';
+import { isUnderTest } from '../../utils/utils';
 
 type BindingSource = { frame: Frame, page: Page };
 
@@ -179,7 +180,7 @@ export class RecorderSupplement {
       this._resume(false).catch(() => {});
     });
 
-    await this._context.extendInjectedScript(recorderSource.source);
+    await this._context.extendInjectedScript(recorderSource.source, { isUnderTest: isUnderTest() });
     await this._context.extendInjectedScript(consoleApiSource.source);
 
     (this._context as any).recorderAppForTest = recorderApp;
@@ -332,7 +333,8 @@ export class RecorderSupplement {
   }
 
   async onAfterCall(metadata: CallMetadata): Promise<void> {
-    this._currentCallsMetadata.delete(metadata);
+    if (!metadata.error)
+      this._currentCallsMetadata.delete(metadata);
     this._pausedCallsMetadata.delete(metadata);
     this._updateUserSources();
     this.updateCallLog([metadata]);
@@ -357,7 +359,7 @@ export class RecorderSupplement {
       }
       if (line) {
         const paused = this._pausedCallsMetadata.has(metadata);
-        source.highlight.push({ line, type: paused ? 'paused' : 'running' });
+        source.highlight.push({ line, type: metadata.error ? 'error' : (paused ? 'paused' : 'running') });
         if (paused)
           source.revealLine = line;
       }
@@ -387,7 +389,7 @@ export class RecorderSupplement {
         status = 'paused';
       if (metadata.error)
         status = 'error';
-      logs.push({ id: metadata.id, messages: metadata.log, title, status });
+      logs.push({ id: metadata.id, messages: metadata.log, title, status, error: metadata.error });
     }
     this._recorderApp?.updateCallLogs(logs);
   }
diff --git a/src/trace/tracer.ts b/src/trace/tracer.ts
@@ -180,7 +180,7 @@ class ContextTracer implements SnapshotterDelegate {
       startTime: metadata.startTime,
       endTime: metadata.endTime,
       logs: metadata.log.slice(),
-      error: metadata.error ? metadata.error.stack : undefined,
+      error: metadata.error,
       snapshots: snapshotsForMetadata(metadata),
     };
     this._appendTraceEvent(event);
diff --git a/src/utils/debugLogger.ts b/src/utils/debugLogger.ts
@@ -16,6 +16,7 @@
 
 import debug from 'debug';
 import fs from 'fs';
+import { EventEmitter } from 'events';
 
 const debugLoggerColorMap = {
   'api': 45, // cyan
@@ -63,10 +64,11 @@ class DebugLogger {
 export const debugLogger = new DebugLogger();
 
 const kLogCount = 50;
-export class RecentLogsCollector {
+export class RecentLogsCollector extends EventEmitter {
   private _logs: string[] = [];
 
   log(message: string) {
+    this.emit('log', message);
     this._logs.push(message);
     if (this._logs.length === kLogCount * 2)
       this._logs.splice(0, kLogCount);
diff --git a/src/web/components/source.css b/src/web/components/source.css
@@ -45,12 +45,18 @@
 }
 
 .source-line-running {
-  background-color: #6fa8dc7f;
+  background-color: #b3dbff7f;
   z-index: 2;
 }
 
 .source-line-paused {
-  background-color: #ffc0cb7f;
-  outline: 1px solid red;
+  background-color: #b3dbff7f;
+  outline: 1px solid #009aff;
+  z-index: 2;
+}
+
+.source-line-error {
+  background-color: #fff0f0;
+  outline: 1px solid #ffd6d6;
   z-index: 2;
 }
diff --git a/src/web/components/source.tsx b/src/web/components/source.tsx
@@ -21,7 +21,7 @@ import '../../third_party/highlightjs/highlightjs/tomorrow.css';
 
 export type SourceHighlight = {
   line: number;
-  type: 'running' | 'paused';
+  type: 'running' | 'paused' | 'error';
 };
 
 export interface SourceProps {
diff --git a/src/web/recorder/recorder.css b/src/web/recorder/recorder.css
@@ -42,15 +42,11 @@
 
 .recorder-log-message {
   flex: none;
-  padding: 3px 12px;
+  padding: 3px 0 3px 36px;
   display: flex;
   align-items: center;
 }
 
-.recorder-log-message-sub-level {
-  padding-left: 40px;
-}
-
 .recorder-log-header {
   color: var(--toolbar-color);
   box-shadow: var(--box-shadow);
@@ -63,18 +59,36 @@
 }
 
 .recorder-log-call {
-  color: var(--toolbar-color);
-  background-color: var(--toolbar-bg-color);
-  border-top: 1px solid #ddd;
-  border-bottom: 1px solid #eee;
+  display: flex;
+  flex: none;
+  flex-direction: column;
+  border-top: 1px solid #eee;
+}
+
+.recorder-log-call-header {
   height: 24px;
   display: flex;
   align-items: center;
-  padding: 0 9px;
-  margin-bottom: 3px;
+  padding: 0 2px;
   z-index: 2;
 }
 
 .recorder-log-call .codicon {
   padding: 0 4px;
 }
+
+.recorder-log .codicon-check {
+  color: #21a945;
+  font-weight: bold;
+}
+
+.recorder-log-call.error {
+  background-color: #fff0f0;
+  border-top: 1px solid #ffd6d6;
+}
+
+.recorder-log-call.error .recorder-log-call-header,
+.recorder-log-message.error,
+.recorder-log .codicon-error {
+  color: red;
+}
diff --git a/src/web/recorder/recorder.tsx b/src/web/recorder/recorder.tsx
@@ -79,12 +79,14 @@ export const Recorder: React.FC<RecorderProps> = ({
         copy(source.text);
       }}></ToolbarButton>
       <ToolbarButton icon='debug-continue' title='Resume' disabled={!paused} onClick={() => {
+        setPaused(false);
         window.dispatch({ event: 'resume' }).catch(() => {});
       }}></ToolbarButton>
       <ToolbarButton icon='debug-pause' title='Pause' disabled={paused} onClick={() => {
         window.dispatch({ event: 'pause' }).catch(() => {});
       }}></ToolbarButton>
       <ToolbarButton icon='debug-step-over' title='Step over' disabled={!paused} onClick={() => {
+        setPaused(false);
         window.dispatch({ event: 'step' }).catch(() => {});
       }}></ToolbarButton>
       <div style={{flex: 'auto'}}></div>
@@ -98,15 +100,18 @@ export const Recorder: React.FC<RecorderProps> = ({
         <div className='recorder-log-header' style={{flex: 'none'}}>Log</div>
         <div className='recorder-log' style={{flex: 'auto'}}>
           {[...log.values()].map(callLog => {
-            return <div className='vbox' style={{flex: 'none'}} key={callLog.id}>
-              <div className='recorder-log-call'>
+            return <div className={`recorder-log-call ${callLog.status}`} key={callLog.id}>
+              <div className='recorder-log-call-header'>
                 <span className={'codicon ' + iconClass(callLog)}></span>{ callLog.title }
               </div>
               { callLog.messages.map((message, i) => {
                 return <div className='recorder-log-message' key={i}>
-                  { message }
+                  { message.trim() }
                 </div>;
               })}
+              { callLog.error ? <div className='recorder-log-message error'>
+                { callLog.error }
+              </div> : undefined }
             </div>
           })}
           <div ref={messagesEndRef}></div>
diff --git a/test/cli/cli.fixtures.ts b/test/cli/cli.fixtures.ts
@@ -34,10 +34,10 @@ type TestFixtures = {
 
 export const fixtures = baseFolio.extend<TestFixtures, WorkerFixtures>();
 
-fixtures.recorder.init(async ({ page, recorderFrame }, runTest) => {
+fixtures.recorder.init(async ({ page, recorderPageGetter }, runTest) => {
   await (page.context() as any)._enableRecorder({ language: 'javascript', startRecording: true });
-  const recorderFrameInstance = await recorderFrame();
-  await runTest(new Recorder(page, recorderFrameInstance));
+  const recorderPage = await recorderPageGetter();
+  await runTest(new Recorder(page, recorderPage));
 });
 
 fixtures.httpServer.init(async ({testWorkerIndex}, runTest) => {
@@ -65,12 +65,12 @@ class Recorder {
   _highlightInstalled: boolean
   _actionReporterInstalled: boolean
   _actionPerformedCallback: Function
-  recorderFrame: any;
+  recorderPage: Page;
   private _text: string = '';
 
-  constructor(page: Page, recorderFrame: any) {
+  constructor(page: Page, recorderPage: Page) {
     this.page = page;
-    this.recorderFrame = recorderFrame;
+    this.recorderPage = recorderPage;
     this._highlightCallback = () => { };
     this._highlightInstalled = false;
     this._actionReporterInstalled = false;
@@ -98,7 +98,7 @@ class Recorder {
   }
 
   async waitForOutput(text: string): Promise<void> {
-    this._text = await this.recorderFrame._evaluateExpression(((text: string) => {
+    this._text = await this.recorderPage.evaluate((text: string) => {
       const w = window as any;
       return new Promise(f => {
         const poll = () => {
@@ -110,7 +110,7 @@ class Recorder {
         };
         setTimeout(poll);
       });
-    }).toString(), true, text, 'main');
+    }, text);
   }
 
   output(): string {
diff --git a/test/pause.spec.ts b/test/pause.spec.ts
diff --git a/test/recorder.fixtures.ts b/test/recorder.fixtures.ts

Original file line number	Diff line number	Diff line change
`@@ -345,8 +345,8 @@ export abstract class BrowserContext extends SdkObject {`
`345`	`345`	`}`
`346`	`346`	`}`
`347`	`347`
`348`		`- async extendInjectedScript(source: string) {`
`349`		`- const installInFrame = (frame: frames.Frame) => frame.extendInjectedScript(source).catch(e => {});`
	`348`	`+ async extendInjectedScript(source: string, arg?: any) {`
	`349`	`+ const installInFrame = (frame: frames.Frame) => frame.extendInjectedScript(source, arg).catch(() => {});`
`350`	`350`	`const installInPage = (page: Page) => {`
`351`	`351`	`page.on(Page.Events.InternalFrameNavigatedToNewDocument, installInFrame);`
`352`	`352`	`return Promise.all(page.frames().map(installInFrame));`