From 72a49ea043c02e0858ee81f7b8f9954756ba82c3 Mon Sep 17 00:00:00 2001
From: Jason Grout <jgrout6@bloomberg.net>
Date: Thu, 1 Aug 2019 11:00:13 -0700
Subject: [PATCH 1/5] Make handling comm messages optional in a kernel
 connection.

The comm message protocol currently has implicit assumptions that only
one kernel connection is handling comm messages. This option allows a
kernel connection to opt out of handling comms.

See https://github.com/jupyter/jupyter_client/issues/263

This fixes the error where ipywidgets stops working if you open a notebook and a console to the same kernel, since the console automatically closes all comms since it does not have the comm target registered.
---
 packages/services/src/kernel/default.ts | 56 +++++++++++++++++++++----
 packages/services/src/kernel/kernel.ts  | 24 +++++++++++
 2 files changed, 72 insertions(+), 8 deletions(-)

diff --git a/packages/services/src/kernel/default.ts b/packages/services/src/kernel/default.ts
index 42d699d0e5be..fbe03867e378 100644
--- a/packages/services/src/kernel/default.ts
+++ b/packages/services/src/kernel/default.ts
@@ -5,7 +5,7 @@ import { URLExt } from '@jupyterlab/coreutils';
 
 import { UUID } from '@phosphor/coreutils';
 
-import { ArrayExt, each, find } from '@phosphor/algorithm';
+import { ArrayExt, each, find, some } from '@phosphor/algorithm';
 
 import { JSONExt, JSONObject, PromiseDelegate } from '@phosphor/coreutils';
 
@@ -61,6 +61,7 @@ export class DefaultKernel implements Kernel.IKernel {
       options.serverSettings || ServerConnection.makeSettings();
     this._clientId = options.clientId || UUID.uuid4();
     this._username = options.username || '';
+    this.handleComms = options.handleComms || true;
 
     void this._readyPromise.promise.then(() => {
       this._sendPending();
@@ -82,6 +83,18 @@ export class DefaultKernel implements Kernel.IKernel {
    */
   readonly serverSettings: ServerConnection.ISettings;
 
+  /**
+   * Handle comm messages
+   *
+   * #### Notes
+   * The comm message protocol currently has implicit assumptions that only
+   * one kernel connection is handling comm messages. This option allows a
+   * kernel connection to opt out of handling comms.
+   *
+   * See https://github.com/jupyter/jupyter_client/issues/263
+   */
+  readonly handleComms: boolean;
+
   /**
    * A signal emitted when the kernel status changes.
    */
@@ -215,12 +228,14 @@ export class DefaultKernel implements Kernel.IKernel {
   /**
    * Clone the current kernel with a new clientId.
    */
-  clone(): Kernel.IKernel {
+  clone(options: Kernel.IOptions = {}): Kernel.IKernel {
     return new DefaultKernel(
       {
         name: this._name,
         username: this._username,
-        serverSettings: this.serverSettings
+        serverSettings: this.serverSettings,
+        handleComms: this.handleComms,
+        ...options
       },
       this._id
     );
@@ -712,11 +727,16 @@ export class DefaultKernel implements Kernel.IKernel {
    *
    * #### Notes
    * If a client-side comm already exists with the given commId, it is returned.
+   * An error is thrown if the kernel does not handle comms.
    */
   connectToComm(
     targetName: string,
     commId: string = UUID.uuid4()
   ): Kernel.IComm {
+    if (!this.handleComms) {
+      throw new Error('Comms are disabled on this kernel connection');
+    }
+
     if (this._comms.has(commId)) {
       return this._comms.get(commId);
     }
@@ -752,6 +772,10 @@ export class DefaultKernel implements Kernel.IKernel {
       msg: KernelMessage.ICommOpenMsg
     ) => void | PromiseLike<void>
   ): void {
+    if (!this.handleComms) {
+      return;
+    }
+
     this._targetRegistry[targetName] = callback;
   }
 
@@ -763,7 +787,7 @@ export class DefaultKernel implements Kernel.IKernel {
    * @param callback - The callback to remove.
    *
    * #### Notes
-   * The comm target is only removed the callback argument matches.
+   * The comm target is only removed if the callback argument matches.
    */
   removeCommTarget(
     targetName: string,
@@ -772,6 +796,10 @@ export class DefaultKernel implements Kernel.IKernel {
       msg: KernelMessage.ICommOpenMsg
     ) => void | PromiseLike<void>
   ): void {
+    if (!this.handleComms) {
+      return;
+    }
+
     if (!this.isDisposed && this._targetRegistry[targetName] === callback) {
       delete this._targetRegistry[targetName];
     }
@@ -1280,13 +1308,19 @@ export class DefaultKernel implements Kernel.IKernel {
           }
           break;
         case 'comm_open':
-          await this._handleCommOpen(msg as KernelMessage.ICommOpenMsg);
+          if (this.handleComms) {
+            await this._handleCommOpen(msg as KernelMessage.ICommOpenMsg);
+          }
           break;
         case 'comm_msg':
-          await this._handleCommMsg(msg as KernelMessage.ICommMsgMsg);
+          if (this.handleComms) {
+            await this._handleCommMsg(msg as KernelMessage.ICommMsgMsg);
+          }
           break;
         case 'comm_close':
-          await this._handleCommClose(msg as KernelMessage.ICommCloseMsg);
+          if (this.handleComms) {
+            await this._handleCommClose(msg as KernelMessage.ICommCloseMsg);
+          }
           break;
         default:
           break;
@@ -1663,7 +1697,13 @@ namespace Private {
       return value.id === model.id;
     });
     if (kernel) {
-      return kernel.clone();
+      // If there is already a kernel connection handling comms, don't handle
+      // them in our clone, since the comm message protocol has implicit
+      // assumptions that only one connection is handling comm messages.
+      // See https://github.com/jupyter/jupyter_client/issues/263
+      const handleComms = !some(runningKernels, k => k.handleComms);
+      const newKernel = kernel.clone({ handleComms });
+      return newKernel;
     }
 
     return new DefaultKernel({ name: model.name, serverSettings }, model.id);
diff --git a/packages/services/src/kernel/kernel.ts b/packages/services/src/kernel/kernel.ts
index 121719918b5b..148e0c01c3b9 100644
--- a/packages/services/src/kernel/kernel.ts
+++ b/packages/services/src/kernel/kernel.ts
@@ -90,6 +90,18 @@ export namespace Kernel {
      */
     readonly ready: Promise<void>;
 
+    /**
+     * Whether the kernel connection handles comm messages.
+     *
+     * #### Notes
+     * The comm message protocol currently has implicit assumptions that only
+     * one kernel connection is handling comm messages. This option allows a
+     * kernel connection to opt out of handling comms.
+     *
+     * See https://github.com/jupyter/jupyter_client/issues/263
+     */
+    handleComms: boolean;
+
     /**
      * Get the kernel spec.
      *
@@ -637,6 +649,18 @@ export namespace Kernel {
      */
     username?: string;
 
+    /**
+     * Whether the kernel connection should handle comm messages
+     *
+     * #### Notes
+     * The comm message protocol currently has implicit assumptions that only
+     * one kernel connection is handling comm messages. This option allows a
+     * kernel connection to opt out of handling comms.
+     *
+     * See https://github.com/jupyter/jupyter_client/issues/263
+     */
+    handleComms?: boolean;
+
     /**
      * The unique identifier for the kernel client.
      */

From ba94eedc72fb59868cffdd21da6c332968ddb2aa Mon Sep 17 00:00:00 2001
From: Jason Grout <jgrout6@bloomberg.net>
Date: Thu, 1 Aug 2019 13:54:15 -0700
Subject: [PATCH 2/5] Fix handleComms default setting so we can actually set it
 to false.

---
 packages/services/src/kernel/default.ts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/packages/services/src/kernel/default.ts b/packages/services/src/kernel/default.ts
index fbe03867e378..39c00b3c421a 100644
--- a/packages/services/src/kernel/default.ts
+++ b/packages/services/src/kernel/default.ts
@@ -61,7 +61,8 @@ export class DefaultKernel implements Kernel.IKernel {
       options.serverSettings || ServerConnection.makeSettings();
     this._clientId = options.clientId || UUID.uuid4();
     this._username = options.username || '';
-    this.handleComms = options.handleComms || true;
+    this.handleComms =
+      options.handleComms === undefined ? true : options.handleComms;
 
     void this._readyPromise.promise.then(() => {
       this._sendPending();

From 4bcfe97c34aa82c330c4b86c71974f536320cf3c Mon Sep 17 00:00:00 2001
From: Jason Grout <jgrout6@bloomberg.net>
Date: Thu, 1 Aug 2019 14:51:07 -0700
Subject: [PATCH 3/5] Consolidate testing utility functions

---
 tests/test-services/src/config/config.spec.ts |  9 +-
 .../test-services/src/contents/index.spec.ts  |  9 +-
 .../test-services/src/kernel/ikernel.spec.ts  |  9 +-
 tests/test-services/src/kernel/kernel.spec.ts |  6 +-
 .../test-services/src/kernel/manager.spec.ts  |  5 +-
 .../src/session/isession.spec.ts              | 10 +-
 .../test-services/src/session/manager.spec.ts |  4 +-
 .../test-services/src/session/session.spec.ts |  3 +-
 .../src/terminal/manager.spec.ts              |  3 +-
 .../src/terminal/terminal.spec.ts             |  4 +-
 tests/test-services/src/utils.spec.ts         |  6 +-
 tests/test-services/src/utils.ts              | 95 -------------------
 testutils/src/index.ts                        | 31 +++++-
 13 files changed, 63 insertions(+), 131 deletions(-)

diff --git a/tests/test-services/src/config/config.spec.ts b/tests/test-services/src/config/config.spec.ts
index f5fa177c09cf..50ecb63e514b 100644
--- a/tests/test-services/src/config/config.spec.ts
+++ b/tests/test-services/src/config/config.spec.ts
@@ -9,12 +9,9 @@ import { JSONObject } from '@phosphor/coreutils';
 
 import { ConfigSection, ConfigWithDefaults } from '@jupyterlab/services';
 
-import {
-  expectFailure,
-  handleRequest,
-  makeSettings,
-  getRequestHandler
-} from '../utils';
+import { expectFailure } from '@jupyterlab/testutils';
+
+import { handleRequest, makeSettings, getRequestHandler } from '../utils';
 
 /**
  * Generate a random config section name.
diff --git a/tests/test-services/src/contents/index.spec.ts b/tests/test-services/src/contents/index.spec.ts
index 63ee387b6c9c..d9be9de1a9b7 100644
--- a/tests/test-services/src/contents/index.spec.ts
+++ b/tests/test-services/src/contents/index.spec.ts
@@ -10,12 +10,9 @@ import {
   ServerConnection
 } from '@jupyterlab/services';
 
-import {
-  DEFAULT_FILE,
-  makeSettings,
-  expectFailure,
-  handleRequest
-} from '../utils';
+import { expectFailure } from '@jupyterlab/testutils';
+
+import { DEFAULT_FILE, makeSettings, handleRequest } from '../utils';
 
 const DEFAULT_DIR: Contents.IModel = {
   name: 'bar',
diff --git a/tests/test-services/src/kernel/ikernel.spec.ts b/tests/test-services/src/kernel/ikernel.spec.ts
index 524c679a7756..313630a85bdf 100644
--- a/tests/test-services/src/kernel/ikernel.spec.ts
+++ b/tests/test-services/src/kernel/ikernel.spec.ts
@@ -11,12 +11,9 @@ import { PromiseDelegate } from '@phosphor/coreutils';
 
 import { Kernel, KernelMessage } from '@jupyterlab/services';
 
-import {
-  expectFailure,
-  KernelTester,
-  handleRequest,
-  testEmission
-} from '../utils';
+import { expectFailure, testEmission } from '@jupyterlab/testutils';
+
+import { KernelTester, handleRequest } from '../utils';
 
 describe('Kernel.IKernel', () => {
   let defaultKernel: Kernel.IKernel;
diff --git a/tests/test-services/src/kernel/kernel.spec.ts b/tests/test-services/src/kernel/kernel.spec.ts
index 2f23df8bb4f5..16d985e48672 100644
--- a/tests/test-services/src/kernel/kernel.spec.ts
+++ b/tests/test-services/src/kernel/kernel.spec.ts
@@ -9,13 +9,13 @@ import { toArray } from '@phosphor/algorithm';
 
 import { Kernel } from '@jupyterlab/services';
 
+import { expectFailure, testEmission } from '@jupyterlab/testutils';
+
 import {
-  expectFailure,
   KernelTester,
   makeSettings,
   PYTHON_SPEC,
-  getRequestHandler,
-  testEmission
+  getRequestHandler
 } from '../utils';
 
 const PYTHON3_SPEC = JSON.parse(JSON.stringify(PYTHON_SPEC));
diff --git a/tests/test-services/src/kernel/manager.spec.ts b/tests/test-services/src/kernel/manager.spec.ts
index 513ecff69588..077257ef2f00 100644
--- a/tests/test-services/src/kernel/manager.spec.ts
+++ b/tests/test-services/src/kernel/manager.spec.ts
@@ -9,12 +9,13 @@ import { JSONExt } from '@phosphor/coreutils';
 
 import { KernelManager, Kernel } from '@jupyterlab/services';
 
+import { testEmission } from '@jupyterlab/testutils';
+
 import {
   PYTHON_SPEC,
   KERNELSPECS,
   handleRequest,
-  makeSettings,
-  testEmission
+  makeSettings
 } from '../utils';
 
 class TestManager extends KernelManager {
diff --git a/tests/test-services/src/session/isession.spec.ts b/tests/test-services/src/session/isession.spec.ts
index 97d17ee8444b..6bde03d648d1 100644
--- a/tests/test-services/src/session/isession.spec.ts
+++ b/tests/test-services/src/session/isession.spec.ts
@@ -13,13 +13,9 @@ import { Kernel, KernelMessage } from '@jupyterlab/services';
 
 import { Session } from '@jupyterlab/services';
 
-import {
-  expectFailure,
-  handleRequest,
-  SessionTester,
-  init,
-  testEmission
-} from '../utils';
+import { expectFailure, testEmission } from '@jupyterlab/testutils';
+
+import { handleRequest, SessionTester, init } from '../utils';
 
 init();
 
diff --git a/tests/test-services/src/session/manager.spec.ts b/tests/test-services/src/session/manager.spec.ts
index 6881f7d3af1c..4e512cd76ba0 100644
--- a/tests/test-services/src/session/manager.spec.ts
+++ b/tests/test-services/src/session/manager.spec.ts
@@ -16,7 +16,9 @@ import {
   Session
 } from '@jupyterlab/services';
 
-import { KERNELSPECS, handleRequest, testEmission } from '../utils';
+import { testEmission } from '@jupyterlab/testutils';
+
+import { KERNELSPECS, handleRequest } from '../utils';
 
 class TestManager extends SessionManager {
   intercept: Kernel.ISpecModels | null = null;
diff --git a/tests/test-services/src/session/session.spec.ts b/tests/test-services/src/session/session.spec.ts
index 9c17bf7406f6..a98d55200ab9 100644
--- a/tests/test-services/src/session/session.spec.ts
+++ b/tests/test-services/src/session/session.spec.ts
@@ -11,8 +11,9 @@ import { ServerConnection } from '@jupyterlab/services';
 
 import { Session } from '@jupyterlab/services';
 
+import { expectFailure } from '@jupyterlab/testutils';
+
 import {
-  expectFailure,
   makeSettings,
   SessionTester,
   createSessionModel,
diff --git a/tests/test-services/src/terminal/manager.spec.ts b/tests/test-services/src/terminal/manager.spec.ts
index f92f1ec87937..7e6195890a0b 100644
--- a/tests/test-services/src/terminal/manager.spec.ts
+++ b/tests/test-services/src/terminal/manager.spec.ts
@@ -10,7 +10,8 @@ import {
   TerminalSession,
   TerminalManager
 } from '@jupyterlab/services';
-import { testEmission } from '../utils';
+
+import { testEmission } from '@jupyterlab/testutils';
 
 describe('terminal', () => {
   let manager: TerminalSession.IManager;
diff --git a/tests/test-services/src/terminal/terminal.spec.ts b/tests/test-services/src/terminal/terminal.spec.ts
index 83e1010644ac..5ad865a5f9ea 100644
--- a/tests/test-services/src/terminal/terminal.spec.ts
+++ b/tests/test-services/src/terminal/terminal.spec.ts
@@ -9,7 +9,9 @@ import { UUID } from '@phosphor/coreutils';
 
 import { TerminalSession } from '@jupyterlab/services';
 
-import { handleRequest, testEmission } from '../utils';
+import { testEmission } from '@jupyterlab/testutils';
+
+import { handleRequest } from '../utils';
 
 describe('terminal', () => {
   let defaultSession: TerminalSession.ISession;
diff --git a/tests/test-services/src/utils.spec.ts b/tests/test-services/src/utils.spec.ts
index 889a909e3f84..03b58ba2aeb8 100644
--- a/tests/test-services/src/utils.spec.ts
+++ b/tests/test-services/src/utils.spec.ts
@@ -7,7 +7,11 @@ import { PromiseDelegate } from '@phosphor/coreutils';
 
 import { Signal } from '@phosphor/signaling';
 
-import { expectFailure, isFulfilled, testEmission } from './utils';
+import {
+  expectFailure,
+  isFulfilled,
+  testEmission
+} from '@jupyterlab/testutils';
 
 describe('test/utils', () => {
   describe('testEmission', () => {
diff --git a/tests/test-services/src/utils.ts b/tests/test-services/src/utils.ts
index b0ca718750bc..081d19d6490c 100644
--- a/tests/test-services/src/utils.ts
+++ b/tests/test-services/src/utils.ts
@@ -15,8 +15,6 @@ import {
 
 import { Response } from 'node-fetch';
 
-import { ISignal, Signal } from '@phosphor/signaling';
-
 import {
   Contents,
   TerminalSession,
@@ -177,35 +175,6 @@ export function handleRequest(item: IService, status: number, body: any) {
   (item.serverSettings as any).fetch = temp;
 }
 
-/**
- * Expect a failure on a promise with the given message.
- */
-export async function expectFailure(
-  promise: Promise<any>,
-  message?: string
-): Promise<void> {
-  let called = false;
-  try {
-    await promise;
-    called = true;
-  } catch (err) {
-    if (message && err.message.indexOf(message) === -1) {
-      throw Error(`Error "${message}" not in: "${err.message}"`);
-    }
-  }
-  if (called) {
-    throw Error(`Failure was not triggered, message was: ${message}`);
-  }
-}
-
-/**
- * Do something in the future ensuring total ordering wrt to Promises.
- */
-export async function doLater(cb: () => void): Promise<void> {
-  await Promise.resolve(void 0);
-  cb();
-}
-
 /**
  * Socket class test rig.
  */
@@ -717,70 +686,6 @@ export class TerminalTester extends SocketTester {
   private _onMessage: (msg: TerminalSession.IMessage) => void = null;
 }
 
-/**
- * Test a single emission from a signal.
- *
- * @param signal - The signal we are listening to.
- * @param find - An optional function to determine which emission to test,
- * defaulting to the first emission.
- * @param test - An optional function which contains the tests for the emission.
- * @param value - An optional value that the promise resolves to if it is
- * successful.
- *
- * @returns a promise that rejects if the function throws an error (e.g., if an
- * expect test doesn't pass), and resolves otherwise.
- *
- * #### Notes
- * The first emission for which the find function returns true will be tested in
- * the test function. If the find function is not given, the first signal
- * emission will be tested.
- *
- * You can test to see if any signal comes which matches a criteria by just
- * giving a find function. You can test the very first signal by just giving a
- * test function. And you can test the first signal matching the find criteria
- * by giving both.
- *
- * The reason this function is asynchronous is so that the thing causing the
- * signal emission (such as a websocket message) can be asynchronous.
- */
-export async function testEmission<T, U, V>(
-  signal: ISignal<T, U>,
-  options: {
-    find?: (a: T, b: U) => boolean;
-    test?: (a: T, b: U) => void;
-    value?: V;
-  }
-): Promise<V> {
-  const done = new PromiseDelegate<V>();
-  const object = {};
-  signal.connect((sender: T, args: U) => {
-    if (!options.find || options.find(sender, args)) {
-      try {
-        Signal.disconnectReceiver(object);
-        if (options.test) {
-          options.test(sender, args);
-        }
-      } catch (e) {
-        done.reject(e);
-      }
-      done.resolve(options.value || undefined);
-    }
-  }, object);
-  return done.promise;
-}
-
-/**
- * Test to see if a promise is fulfilled.
- *
- * @returns true if the promise is fulfilled (either resolved or rejected), and
- * false if the promise is still pending.
- */
-export async function isFulfilled<T>(p: PromiseLike<T>): Promise<boolean> {
-  const x = Object.create(null);
-  const result = await Promise.race([p, x]).catch(() => false);
-  return result !== x;
-}
-
 /**
  * Make a new type with the given keys declared as optional.
  *
diff --git a/testutils/src/index.ts b/testutils/src/index.ts
index cdb4c462d9b4..e7e50368c500 100644
--- a/testutils/src/index.ts
+++ b/testutils/src/index.ts
@@ -48,7 +48,7 @@ export { defaultRenderMime } from './rendermime';
  * The reason this function is asynchronous is so that the thing causing the
  * signal emission (such as a websocket message) can be asynchronous.
  */
-export function testEmission<T, U, V>(
+export async function testEmission<T, U, V>(
   signal: ISignal<T, U>,
   options: {
     find?: (a: T, b: U) => boolean;
@@ -74,6 +74,35 @@ export function testEmission<T, U, V>(
   return done.promise;
 }
 
+/**
+ * Expect a failure on a promise with the given message.
+ */
+export async function expectFailure(
+  promise: Promise<any>,
+  message?: string
+): Promise<void> {
+  let called = false;
+  try {
+    await promise;
+    called = true;
+  } catch (err) {
+    if (message && err.message.indexOf(message) === -1) {
+      throw Error(`Error "${message}" not in: "${err.message}"`);
+    }
+  }
+  if (called) {
+    throw Error(`Failure was not triggered, message was: ${message}`);
+  }
+}
+
+/**
+ * Do something in the future ensuring total ordering with respect to promises.
+ */
+export async function doLater(cb: () => void): Promise<void> {
+  await Promise.resolve(void 0);
+  cb();
+}
+
 /**
  * Convert a signal into an array of promises.
  *

From b798bcd5d88d1f592a07c72616adbc1060d4e3c2 Mon Sep 17 00:00:00 2001
From: Jason Grout <jgrout6@bloomberg.net>
Date: Thu, 1 Aug 2019 15:56:17 -0700
Subject: [PATCH 4/5] Add testing of handleComms option, fix errors.

---
 packages/services/src/kernel/default.ts       |  5 ++-
 tests/test-services/src/kernel/comm.spec.ts   | 42 +++++++++++++++++++
 tests/test-services/src/kernel/kernel.spec.ts | 16 +++++++
 testutils/src/index.ts                        | 14 ++++++-
 4 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/packages/services/src/kernel/default.ts b/packages/services/src/kernel/default.ts
index 39c00b3c421a..266b4828e8c7 100644
--- a/packages/services/src/kernel/default.ts
+++ b/packages/services/src/kernel/default.ts
@@ -1702,7 +1702,10 @@ namespace Private {
       // them in our clone, since the comm message protocol has implicit
       // assumptions that only one connection is handling comm messages.
       // See https://github.com/jupyter/jupyter_client/issues/263
-      const handleComms = !some(runningKernels, k => k.handleComms);
+      const handleComms = !some(
+        runningKernels,
+        k => k.id === model.id && k.handleComms
+      );
       const newKernel = kernel.clone({ handleComms });
       return newKernel;
     }
diff --git a/tests/test-services/src/kernel/comm.spec.ts b/tests/test-services/src/kernel/comm.spec.ts
index c1fe9698b050..346ca807eb78 100644
--- a/tests/test-services/src/kernel/comm.spec.ts
+++ b/tests/test-services/src/kernel/comm.spec.ts
@@ -7,6 +7,8 @@ import { PromiseDelegate } from '@phosphor/coreutils';
 
 import { KernelMessage, Kernel } from '@jupyterlab/services';
 
+import { isFulfilled } from '@jupyterlab/testutils';
+
 import { init } from '../utils';
 
 // Initialize fetch override.
@@ -82,6 +84,15 @@ describe('jupyter.services - Comm', () => {
         const comm2 = kernel.connectToComm('test', '1234');
         expect(comm).to.equal(comm2);
       });
+
+      it('should throw an error when the kernel does not handle comms', async () => {
+        const kernel2 = await Kernel.startNew({
+          name: 'ipython',
+          handleComms: false
+        });
+        expect(kernel2.handleComms).to.be.false;
+        expect(() => kernel2.connectToComm('test', '1234')).to.throw();
+      });
     });
 
     describe('#registerCommTarget()', () => {
@@ -105,6 +116,37 @@ describe('jupyter.services - Comm', () => {
         kernel.removeCommTarget('test', hook);
         comm.dispose();
       });
+
+      it('should do nothing if the kernel does not handle comms', async () => {
+        const promise = new PromiseDelegate<
+          [Kernel.IComm, KernelMessage.ICommOpenMsg]
+        >();
+        const hook = (comm: Kernel.IComm, msg: KernelMessage.ICommOpenMsg) => {
+          promise.resolve([comm, msg]);
+        };
+        const kernel2 = await Kernel.startNew({
+          name: 'ipython',
+          handleComms: false
+        });
+        kernel2.registerCommTarget('test', hook);
+
+        // Request the comm creation.
+        await kernel2.requestExecute({ code: SEND }, true).done;
+
+        // The promise above should not be fulfilled, even after a short delay.
+        expect(await isFulfilled(promise.promise, 300)).to.be.false;
+
+        // The kernel comm has not been closed - we just completely ignored it.
+        let reply = await kernel2.requestExecute(
+          { code: `assert comm._closed is False` },
+          true
+        ).done;
+        // If the assert was false, we would get an 'error' status
+        expect(reply.content.status).to.equal('ok');
+
+        // Clean up
+        kernel2.removeCommTarget('test', hook);
+      });
     });
 
     describe('#commInfo()', () => {
diff --git a/tests/test-services/src/kernel/kernel.spec.ts b/tests/test-services/src/kernel/kernel.spec.ts
index 16d985e48672..8f83fd16ad37 100644
--- a/tests/test-services/src/kernel/kernel.spec.ts
+++ b/tests/test-services/src/kernel/kernel.spec.ts
@@ -164,6 +164,22 @@ describe('kernel', () => {
       expect(kernel.id).to.equal(id);
       kernel.dispose();
     });
+
+    it('should turn off comm handling in the new connection if it was enabled in first kernel', () => {
+      expect(defaultKernel.handleComms).to.be.true;
+      const kernel = Kernel.connectTo(defaultKernel.model);
+      expect(kernel.handleComms).to.be.false;
+      kernel.dispose();
+    });
+
+    it('should turn on comm handling in the new connection if it was disabled in all other connections', async () => {
+      const kernel = await Kernel.startNew({ handleComms: false });
+      expect(kernel.handleComms).to.be.false;
+      const kernel2 = Kernel.connectTo(defaultKernel.model);
+      expect(kernel2.handleComms).to.be.true;
+      kernel.dispose();
+      kernel2.dispose();
+    });
   });
 
   describe('Kernel.shutdown()', () => {
diff --git a/testutils/src/index.ts b/testutils/src/index.ts
index e7e50368c500..3dc36ccfd3bd 100644
--- a/testutils/src/index.ts
+++ b/testutils/src/index.ts
@@ -153,12 +153,22 @@ export function signalToPromise<T, U>(signal: ISignal<T, U>): Promise<[T, U]> {
 /**
  * Test to see if a promise is fulfilled.
  *
+ * @param delay - optional delay in milliseconds before checking
  * @returns true if the promise is fulfilled (either resolved or rejected), and
  * false if the promise is still pending.
  */
-export async function isFulfilled<T>(p: PromiseLike<T>): Promise<boolean> {
+export async function isFulfilled<T>(
+  p: PromiseLike<T>,
+  delay = 0
+): Promise<boolean> {
   let x = Object.create(null);
-  let result = await Promise.race([p, x]).catch(() => false);
+  let race: any;
+  if (delay > 0) {
+    race = sleep(delay, x);
+  } else {
+    race = x;
+  }
+  let result = await Promise.race([p, race]).catch(() => false);
   return result !== x;
 }
 

From 976fbb0eaa8adc6f50f85d1c43eddb5b844252b5 Mon Sep 17 00:00:00 2001
From: Jason Grout <jgrout6@bloomberg.net>
Date: Thu, 1 Aug 2019 16:59:00 -0700
Subject: [PATCH 5/5] Make a comm test more robust.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It seems that sometimes the defaultKernel wasn’t found in the private list of running kernels…
---
 tests/test-services/src/kernel/kernel.spec.ts | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/test-services/src/kernel/kernel.spec.ts b/tests/test-services/src/kernel/kernel.spec.ts
index 8f83fd16ad37..c67df74c3ad8 100644
--- a/tests/test-services/src/kernel/kernel.spec.ts
+++ b/tests/test-services/src/kernel/kernel.spec.ts
@@ -165,11 +165,13 @@ describe('kernel', () => {
       kernel.dispose();
     });
 
-    it('should turn off comm handling in the new connection if it was enabled in first kernel', () => {
-      expect(defaultKernel.handleComms).to.be.true;
-      const kernel = Kernel.connectTo(defaultKernel.model);
-      expect(kernel.handleComms).to.be.false;
+    it('should turn off comm handling in the new connection if it was enabled in first kernel', async () => {
+      const kernel = await Kernel.startNew();
+      expect(kernel.handleComms).to.be.true;
+      const kernel2 = Kernel.connectTo(kernel.model);
+      expect(kernel2.handleComms).to.be.false;
       kernel.dispose();
+      kernel2.dispose();
     });
 
     it('should turn on comm handling in the new connection if it was disabled in all other connections', async () => {