fix: test errors and flakes

- EPIPE error sometimes being thrown in streams - Whatever Chromium is installed by default on Catalina seems terribly slow--grab playwright and use its installation - Clean up some noisy console errors resulting from an extra error type and leaks in the undisposed binder
microsoft · Apr 7, 2020 · ea9d6a4 · ea9d6a4
1 parent 371d216
commit ea9d6a4
Show file tree

Hide file tree

Showing 11 changed files with 127 additions and 113 deletions.
diff --git a/.vscode/launch.json b/.vscode/launch.json
@@ -102,8 +102,8 @@
     },
 		{
 			"name": "Run Tests",
-			"type": "extensionHost",
-			"request": "launch",
+			"type": "pwa-extensionHost",
+      "request": "launch",
       "runtimeExecutable": "${execPath}",
       "skipFiles": ["<node_internals>/**"],
 			"args": [

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -93,7 +93,6 @@
     "@types/mkdirp": "^1.0.0",
     "@types/mocha": "^7.0.2",
     "@types/node": "^13.11.0",
-    "@types/puppeteer": "^2.0.1",
     "@types/sinon": "^9.0.0",
     "@types/split2": "^2.1.6",
     "@types/stream-buffers": "^3.0.3",
@@ -128,8 +127,8 @@
     "mocha-multi-reporters": "^1.1.7",
     "npm-run-all": "^4.1.5",
     "nyc": "^15.0.1",
+    "playwright": "^0.12.1",
     "prettier": "^2.0.4",
-    "puppeteer": "^2.1.1",
     "sinon": "^9.0.1",
     "stream-buffers": "^3.0.2",
     "ts-node": "^8.8.2",

diff --git a/src/adapter/breakpoints/breakpointBase.ts b/src/adapter/breakpoints/breakpointBase.ts
@@ -380,8 +380,8 @@ export abstract class Breakpoint {
    * requests to avoid triggering any logpoint breakpoints multiple times,
    * as would happen if we set a breakpoint both by script and URL.
    */
-  private hasSetOnLocation(script: Partial<Script>, lineColumn: LineColumn): boolean {
-    return this.cdpBreakpoints.some(
+  private hasSetOnLocation(script: Partial<Script>, lineColumn: LineColumn) {
+    return this.cdpBreakpoints.find(
       bp =>
         (script.url &&
           isSetByUrl(bp.args) &&
@@ -396,7 +396,13 @@ export abstract class Breakpoint {
 
   private async _setByUrl(thread: Thread, url: string, lineColumn: LineColumn): Promise<void> {
     lineColumn = base1To0(lineColumn);
-    if (this.hasSetOnLocation({ url }, lineColumn)) {
+
+    const previous = this.hasSetOnLocation({ url }, lineColumn);
+    if (previous) {
+      if (previous.state === CdpReferenceState.Pending) {
+        await previous.done;
+      }
+
       return;
     }
 
@@ -415,7 +421,12 @@ export abstract class Breakpoint {
     lineColumn = base1To0(lineColumn);
 
     // Avoid setting duplicate breakpoints
-    if (this.hasSetOnLocation(script, lineColumn)) {
+    const previous = this.hasSetOnLocation(script, lineColumn);
+    if (previous) {
+      if (previous.state === CdpReferenceState.Pending) {
+        await previous.done;
+      }
+
       return;
     }
 

diff --git a/src/adapter/threads.ts b/src/adapter/threads.ts
@@ -414,7 +414,7 @@ export class Thread implements IVariableStoreDelegate {
 
     if (response.exceptionDetails) {
       const formattedException = await this._formatException(response.exceptionDetails);
-      throw new errors.ExternalError(formattedException.output);
+      throw new errors.ProtocolError(errors.replError(formattedException.output));
     } else {
       const contextName =
         this._selectedContext && this.defaultExecutionContext() !== this._selectedContext

diff --git a/src/binder.ts b/src/binder.ts
@@ -150,7 +150,11 @@ export class Binder implements IDisposable {
   }
 
   private async _disconnect() {
-    await Promise.all([...this.getLaunchers()].map(l => l.disconnect()));
+    if (!this._launchers) {
+      return;
+    }
+
+    await Promise.all([...this._launchers].map(l => l.disconnect()));
 
     const didTerminate = () => !this.targetList.length && this._terminationCount === 0;
     if (didTerminate()) {

diff --git a/src/cdp/rawPipeTransport.ts b/src/cdp/rawPipeTransport.ts
@@ -33,13 +33,14 @@ export class RawPipeTransport implements ITransport {
     this.pipeRead?.destroy();
     this.streams.write.removeListener('end', this.onceEnded);
     this.streams.write.removeListener('error', this.onWriteError);
+    // Suppress pipe errors, e.g. EPIPE when pipe is destroyed with buffered data
+    this.streams.write.on('error', () => undefined);
     this.streams.write.end();
     this.streams = undefined;
     this.endEmitter.fire();
   });
 
   private readonly onWriteError = (error: Error) => {
-    // Suppress pipe errors, e.g. EPIPE when pipe is destroyed with buffered data
     this.logger.error(LogTag.Internal, 'pipeWrite error', { error });
   };
 
@@ -54,10 +55,10 @@ export class RawPipeTransport implements ITransport {
     const read = pipeRead || pipeWrite;
     this.streams = {
       read: read
+        .on('error', error => this.logger.error(LogTag.Internal, 'pipeRead error', { error }))
         .pipe(split('\0'))
         .on('data', json => this.messageEmitter.fire([json, process.hrtime.bigint()]))
-        .on('end', this.onceEnded)
-        .on('error', error => this.logger.error(LogTag.Internal, 'pipeRead error', { error })),
+        .on('end', this.onceEnded),
       write: pipeWrite.on('end', this.onceEnded).on('error', this.onWriteError),
     };
   }

diff --git a/src/dap/connection.ts b/src/dap/connection.ts
@@ -6,7 +6,7 @@ import Dap from './api';
 
 import { ITelemetryReporter } from '../telemetry/telemetryReporter';
 import { ILogger } from '../common/logging';
-import { isDapError, isExternalError, ProtocolError } from './errors';
+import { isDapError, ProtocolError } from './errors';
 import { Message, IDapTransport } from './transport';
 import { IDisposable } from '../common/disposable';
 import { getDeferred } from '../common/promiseUtil';
@@ -159,7 +159,7 @@ export default class Connection {
       };
 
       try {
-        const callback = msg.command && this._requestHandlers.get(msg.command);
+        const callback = this._requestHandlers.get(msg.command);
         if (!callback) {
           console.error(`Unknown request: ${msg.command}`);
         } else {
@@ -184,10 +184,6 @@ export default class Connection {
           Number(process.hrtime.bigint() - receivedTime) / 1e6,
         );
       } catch (e) {
-        const format = isExternalError(e)
-          ? e.message
-          : `Error processing ${msg.command}: ${e.stack || e.message}`;
-
         if (e instanceof ProtocolError) {
           this._send({
             ...response,
@@ -202,7 +198,7 @@ export default class Connection {
             body: {
               error: {
                 id: 9221,
-                format,
+                format: `Error processing ${msg.command}: ${e.stack || e.message}`,
                 showUser: false,
                 sendTelemetry: false,
               },