Browse Source

Cleanup based on review comments by @ian-r-rose

Jason Grout 6 years ago
parent
commit
4a30e0f035

+ 3 - 3
packages/services/src/kernel/default.ts

@@ -1119,7 +1119,7 @@ class DefaultKernel implements Kernel.IKernel {
   private _displayIdToParentIds = new Map<string, string[]>();
   private _msgIdToDisplayIds = new Map<string, string[]>();
   private _terminated = new Signal<this, void>(this);
-  private _msgChain = Promise.resolve();
+  private _msgChain: Promise<void> | null = Promise.resolve();
   private _noOp = () => { /* no-op */};
 }
 
@@ -1428,7 +1428,7 @@ namespace Private {
     // We might want to move the handleRestart to after we get the response back
 
     // Handle the restart on all of the kernels with the same id.
-    each(runningKernels.slice(), k => {
+    each(runningKernels, k => {
       if (k.id === kernel.id) {
         k.handleRestart();
       }
@@ -1440,7 +1440,7 @@ namespace Private {
     let data = await response.json();
     validate.validateModel(data);
     // Reconnect the other kernels asynchronously, but don't wait for them.
-    each(runningKernels.slice(), k => {
+    each(runningKernels, k => {
       if (k !== kernel && k.id === kernel.id) {
         k.reconnect();
       }

+ 1 - 1
packages/services/src/kernel/future.ts

@@ -169,7 +169,7 @@ class KernelFutureHandler extends DisposableDelegate implements Kernel.IFuture {
       this._done.promise.catch(() => { /* no-op */ });
 
 
-      // TODO: Check tests for any kernels that are disposed before done
+      // TODO: Uncomment the following logging code, and check for any tests that trigger it.
       // let status = [];
       // if (!this._testFlag(Private.KernelFutureFlag.GotIdle)) {
       //   status.push('idle');

+ 6 - 7
packages/services/test/src/kernel/kernel.spec.ts

@@ -108,16 +108,15 @@ describe('kernel', () => {
       });
     });
 
+    // TODO: fix this test. The idea here is that if a kernel immediately
+    // replies that it is dead, we still construct the kernel object and give it
+    // a chance to handle the dead status. When is this going to happen? A
+    // kernel typically doesn't immediately send its status, does it? I suppose
+    // it should - if we are connecting to an existing kernel, it would be nice
+    // to know right off if it's busy/dead/etc.
     it.skip('should still start if the kernel dies', (done) => {
       tester = new KernelTester();
       tester.initialStatus = 'dead';
-      // const kernel = await tester.start();
-      // TODO: The idea here is that if a kernel immediately replies that it is dead,
-      // we still construct the kernel object and give it a chance to handle the
-      // dead status. When is this going to happen? A kernel typically doesn't
-      // immediately send its status, does it? I suppose it should - if we are
-      // connecting to an existing kernel, it would be nice to know right off if
-      // it's busy/dead/etc.
       tester.start().then(kernel => {
         kernel.statusChanged.connect((sender, state) => {
           if (state === 'dead') {

+ 8 - 5
packages/services/test/src/utils.ts

@@ -687,7 +687,8 @@ class TerminalTester extends SocketTester {
  * @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.
+ * @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.
@@ -698,9 +699,12 @@ class TerminalTester extends SocketTester {
  * 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.
+ * 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: {
@@ -719,7 +723,6 @@ async function testEmission<T, U, V>(signal: ISignal<T, U>, options: {
         }
       } catch (e) {
         done.reject(e);
-        // throw e;
       }
       done.resolve(options.value || undefined);
     }