Fix conversation context (and add tests)

.nycrc.json

@@ -3,7 +3,8 @@
     "src/**/*.ts"
   ],
   "exclude": [
-    "**/*.spec.ts"
+    "**/*.spec.ts",
+    "src/test-helpers.ts"
   ],
   "reporter": ["lcov"],
   "extension": [

src/App.spec.ts

@@ -1,13 +1,13 @@
-// tslint:disable:ter-prefer-arrow-callback typedef no-implicit-dependencies no-this-assignment
+// tslint:disable:no-implicit-dependencies
 import 'mocha';
 import { EventEmitter } from 'events';
 import sinon, { SinonSpy } from 'sinon';
 import { assert } from 'chai';
+import { Override, mergeOverrides, createFakeLogger, delay } from './test-helpers';
 import rewiremock from 'rewiremock';
 import { ErrorCode } from './errors';
 import { Receiver, ReceiverEvent, SayFn, NextMiddleware } from './types';
 import { ConversationStore } from './conversation-store';
-import { Logger } from '@slack/logger';
 
 describe('App', () => {
   describe('constructor', () => {
@@ -567,12 +567,6 @@ async function importApp(
 }
 
 // Composable overrides
-interface Override {
-  [packageName: string]: {
-    [exportName: string]: any;
-  };
-}
-
 function withNoopWebClient(): Override {
   return {
     '@slack/web-api': {
@@ -589,7 +583,6 @@ function withNoopAppMetadata(): Override {
   };
 }
 
-// TODO: see if we can use a partial type for the return value
 function withSuccessfulBotUserFetchingWebClient(botId: string, botUserId: string): Override {
   return {
     '@slack/web-api': {
@@ -639,30 +632,6 @@ function withConversationContext(spy: SinonSpy): Override {
   };
 }
 
-function mergeOverrides(...overrides: Override[]): Override {
-  let currentOverrides: Override = {};
-  for (const override of overrides) {
-    currentOverrides = mergeObjProperties(currentOverrides, override);
-  }
-  return currentOverrides;
-}
-
-function mergeObjProperties(first: Override, second: Override): Override {
-  const merged: Override = {};
-  const props = Object.keys(first).concat(Object.keys(second));
-  for (const prop of props) {
-    if (second[prop] === undefined && first[prop] !== undefined) {
-      merged[prop] = first[prop];
-    } else if (first[prop] === undefined && second[prop] !== undefined) {
-      merged[prop] = second[prop];
-    } else {
-      // second always overwrites the first
-      merged[prop] = { ...first[prop], ...second[prop] };
-    }
-  }
-  return merged;
-}
-
 // Fakes
 type FakeReceiver = SinonSpy & EventEmitter & {
   start: SinonSpy<Parameters<Receiver['start']>, ReturnType<Receiver['start']>>;
@@ -679,28 +648,6 @@ function createFakeReceiver(
   return mock as FakeReceiver;
 }
 
-interface FakeLogger extends Logger {
-  setLevel: SinonSpy<Parameters<Logger['setLevel']>, ReturnType<Logger['setLevel']>>;
-  setName: SinonSpy<Parameters<Logger['setName']>, ReturnType<Logger['setName']>>;
-  debug: SinonSpy<Parameters<Logger['debug']>, ReturnType<Logger['debug']>>;
-  info: SinonSpy<Parameters<Logger['info']>, ReturnType<Logger['info']>>;
-  warn: SinonSpy<Parameters<Logger['warn']>, ReturnType<Logger['warn']>>;
-  error: SinonSpy<Parameters<Logger['error']>, ReturnType<Logger['error']>>;
-}
-
-function createFakeLogger(): FakeLogger {
-  return {
-    // NOTE: the two casts are because of a TypeScript inconsistency with tuple types and any[]. all tuple types
-    // should be assignable to any[], but TypeScript doesn't think so.
-    setLevel: sinon.fake() as SinonSpy<Parameters<Logger['setLevel']>, ReturnType<Logger['setLevel']>>,
-    setName: sinon.fake() as SinonSpy<Parameters<Logger['setName']>, ReturnType<Logger['setName']>>,
-    debug: sinon.fake(),
-    info: sinon.fake(),
-    warn: sinon.fake(),
-    error: sinon.fake(),
-  };
-}
-
 // Dummies (values that have no real behavior but pass through the system opaquely)
 function createDummyReceiverEvent(): ReceiverEvent {
   // NOTE: this is a degenerate ReceiverEvent that would successfully pass through the App. it happens to look like a
@@ -719,10 +666,5 @@ function createDummyReceiverEvent(): ReceiverEvent {
 const noop = () => { }; // tslint:disable-line:no-empty
 const noopMiddleware = ({ next }: { next: NextMiddleware; }) => { next(); };
 const noopAuthorize = (() => Promise.resolve({}));
-function delay(ms: number = 0) {
-  return new Promise((resolve) => {
-    setTimeout(resolve, ms);
-  });
-}
 
 // TODO: swap out rewiremock for proxyquire to see if it saves execution time

src/ExpressReceiver.ts

@@ -135,8 +135,6 @@ export default class ExpressReceiver extends EventEmitter implements Receiver {
   }
 }
 
-// TODO: respond to url_verification, and also help a beginner set up Events API (maybe adopt the CLI verify tool)
-
 const respondToSslCheck: RequestHandler = (req, res, next) => {
   if (req.body && req.body.ssl_check) {
     res.send();

src/conversation-store.spec.ts

@@ -0,0 +1,262 @@
+// tslint:disable:no-implicit-dependencies
+import 'mocha';
+import { assert, AssertionError } from 'chai';
+import sinon, { SinonSpy } from 'sinon';
+import { Override, createFakeLogger, delay } from './test-helpers';
+import rewiremock from 'rewiremock';
+import { ConversationStore } from './conversation-store';
+import { AnyMiddlewareArgs, NextMiddleware, Context } from './types';
+
+describe('conversationContext middleware', () => {
+  it('should forward events that have no conversation ID', async () => {
+    // Arrange
+    // conversationId property is omitted from return value
+    const fakeGetTypeAndConversation = sinon.fake.returns({});
+    const fakeStore = createFakeStore();
+    const fakeLogger = createFakeLogger();
+    const fakeNext = sinon.fake();
+    const dummyContext: DummyContext<void> = {};
+    const { conversationContext } = await importConversationStore(
+      withGetTypeAndConversation(fakeGetTypeAndConversation),
+    );
+    const fakeArgs = { body: {}, context: dummyContext, next: fakeNext } as unknown as MiddlewareArgs;
+
+    // Act
+    const middleware = conversationContext(fakeStore, fakeLogger);
+    middleware(fakeArgs);
+
+    // Assert
+    assert(fakeLogger.debug.called);
+    assert(fakeNext.called);
+    assert.notProperty(dummyContext, 'updateConversation');
+    assert.notProperty(dummyContext, 'conversation');
+  });
+
+  // TODO: test that expiresAt is passed through on calls to store.set
+
+  it('should add to the context for events within a conversation that was not previously stored', async () => {
+    // Arrange
+    const dummyConversationState = Symbol();
+    const dummyConversationId = 'CONVERSATION_ID';
+    const dummyStoreSetResult = Symbol();
+    const fakeGetTypeAndConversation = sinon.fake.returns({ conversationId: dummyConversationId });
+    const fakeStore = createFakeStore(
+      sinon.fake.rejects(new Error('Test conversation missing')),
+      sinon.fake.resolves(dummyStoreSetResult),
+    );
+    const fakeLogger = createFakeLogger();
+    const { fn: next, promise: onNextFirstCall } = wrapToResolveOnFirstCall(assertions);
+    const dummyContext: DummyContext<symbol> = {};
+    const { conversationContext } = await importConversationStore(
+      withGetTypeAndConversation(fakeGetTypeAndConversation),
+    );
+    const fakeArgs = { next, body: {}, context: dummyContext } as unknown as MiddlewareArgs;
+
+    // Act
+    const middleware = conversationContext(fakeStore, fakeLogger);
+    middleware(fakeArgs);
+
+    // Assert
+    async function assertions(...args: any[]): Promise<void> {
+      assert.notExists(args[0]);
+      assert.notProperty(dummyContext, 'conversation');
+      if (dummyContext.updateConversation !== undefined) {
+        const result = await dummyContext.updateConversation(dummyConversationState);
+        assert.equal(result, dummyStoreSetResult);
+        assert(fakeStore.set.calledOnce);
+        assert(fakeStore.set.calledWith(dummyConversationId, dummyConversationState));
+      } else {
+        assert.fail();
+      }
+    }
+    return onNextFirstCall;
+  });
+
+  it('should add to the context for events within a conversation that was previously stored', async () => {
+    // Arrange
+    const dummyConversationState = Symbol();
+    const dummyConversationId = 'CONVERSATION_ID';
+    const dummyStoreSetResult = Symbol();
+    const fakeGetTypeAndConversation = sinon.fake.returns({ conversationId: dummyConversationId });
+    const fakeStore = createFakeStore(
+      sinon.fake.resolves(dummyConversationState),
+      sinon.fake.resolves(dummyStoreSetResult),
+    );
+    const fakeLogger = createFakeLogger();
+    const { fn: next, promise: onNextFirstCall } = wrapToResolveOnFirstCall(assertions);
+    const dummyContext: DummyContext<symbol> = {};
+    const { conversationContext } = await importConversationStore(
+      withGetTypeAndConversation(fakeGetTypeAndConversation),
+    );
+    const fakeArgs = { next, body: {}, context: dummyContext } as unknown as MiddlewareArgs;
+
+    // Act
+    const middleware = conversationContext(fakeStore, fakeLogger);
+    middleware(fakeArgs);
+
+    // Assert
+    async function assertions(...args: any[]): Promise<void> {
+      assert.notExists(args[0]);
+      assert.equal(dummyContext.conversation, dummyConversationState);
+      if (dummyContext.updateConversation !== undefined) {
+        const newDummyConversationState = Symbol();
+        const result = await dummyContext.updateConversation(newDummyConversationState);
+        assert.equal(result, dummyStoreSetResult);
+        assert(fakeStore.set.calledOnce);
+        assert(fakeStore.set.calledWith(dummyConversationId, newDummyConversationState));
+      } else {
+        assert.fail();
+      }
+    }
+    return onNextFirstCall;
+  });
+});
+
+describe('MemoryStore', () => {
+  describe('constructor', () => {
+    it('should initialize successfully', async () => {
+      // Arrange
+      const { MemoryStore } = await importConversationStore();
+
+      // Act
+      const store = new MemoryStore();
+
+      // Assert
+      assert.isOk(store);
+    });
+  });
+
+  // NOTE: there's no good way to fetch the contents of the map that backs the state with an override, so instead we use
+  // the public API once again. as a consequence, this is not a pure unit test of a single method, but it does verify
+  // the expected behavior when looking at set and get as one unit.
+  describe('#set and #get', () => {
+    it('should store conversation state', async () => {
+      // Arrange
+      const dummyConversationState = Symbol();
+      const dummyConversationId = 'CONVERSATION_ID';
+      const { MemoryStore } = await importConversationStore();
+
+      // Act
+      const store = new MemoryStore();
+      await store.set(dummyConversationId, dummyConversationState);
+      const actualConversationState = await store.get(dummyConversationId);
+
+      // Assert
+      assert.equal(actualConversationState, dummyConversationState);
+    });
+
+    it('should reject lookup of conversation state when the conversation is not stored', async () => {
+      // Arrange
+      const { MemoryStore } = await importConversationStore();
+
+      // Act
+      const store = new MemoryStore();
+      try {
+        await store.get('CONVERSATION_ID');
+        assert.fail();
+      } catch (error) {
+        // Assert
+        assert.instanceOf(error, Error);
+        assert.notInstanceOf(error, AssertionError);
+      }
+    });
+
+    it('should reject lookup of conversation state when the conversation is expired', async () => {
+      // Arrange
+      const dummyConversationId = 'CONVERSATION_ID';
+      const dummyConversationState = Symbol();
+      const expiresInMs = 5;
+      const { MemoryStore } = await importConversationStore();
+
+      // Act
+      const store = new MemoryStore();
+      await store.set(dummyConversationId, dummyConversationState, Date.now() + expiresInMs);
+      await delay(expiresInMs * 2);
+      try {
+        await store.get(dummyConversationId);
+        assert.fail();
+      } catch (error) {
+        // Assert
+        assert.instanceOf(error, Error);
+        assert.notInstanceOf(error, AssertionError);
+      }
+    });
+  });
+});
+
+/* Testing Harness */
+
+type MiddlewareArgs = AnyMiddlewareArgs & { next: NextMiddleware, context: Context };
+
+interface DummyContext<ConversationState> {
+  conversation?: ConversationState;
+  updateConversation?: (c: ConversationState) => Promise<unknown>;
+}
+
+// Loading the system under test using overrides
+async function importConversationStore(
+  overrides: Override = {},
+): Promise<typeof import('./conversation-store')> {
+  return rewiremock.module(() => import('./conversation-store'), overrides);
+}
+
+// Composable overrides
+function withGetTypeAndConversation(spy: SinonSpy): Override {
+  return {
+    './helpers': {
+      getTypeAndConversation: spy,
+    },
+  };
+}
+
+// Fakes
+interface FakeStore extends ConversationStore {
+  set: SinonSpy<Parameters<ConversationStore['set']>, ReturnType<ConversationStore['set']>>;
+  get: SinonSpy<Parameters<ConversationStore['get']>, ReturnType<ConversationStore['get']>>;
+}
+
+function createFakeStore(
+  getSpy: SinonSpy = sinon.fake.resolves(undefined),
+  setSpy: SinonSpy = sinon.fake.resolves({}),
+): FakeStore {
+  return {
+    set: setSpy as SinonSpy<Parameters<ConversationStore['set']>, ReturnType<ConversationStore['set']>>,
+    get: getSpy as SinonSpy<Parameters<ConversationStore['get']>, ReturnType<ConversationStore['get']>>,
+  };
+}
+
+// Utility functions
+function wrapToResolveOnFirstCall<T extends (...args: any[]) => void>(
+  original: T,
+  timeoutMs: number = 1000,
+): { fn: (...args: Parameters<T>) => Promise<void>; promise: Promise<void>; } {
+  // tslint:disable-next-line:no-empty
+  let firstCallResolve: (value?: void | PromiseLike<void>) => void = () => { };
+  let firstCallReject: (reason?: any) => void = () => { }; // tslint:disable-line:no-empty
+
+  const firstCallPromise: Promise<void> = new Promise((resolve, reject) => {
+    firstCallResolve = resolve;
+    firstCallReject = reject;
+  });
+
+  const wrapped = async function (this: ThisParameterType<T>, ...args: Parameters<T>): Promise<void> {
+    try {
+      await original.call(this, ...args);
+      firstCallResolve();
+    } catch (error) {
+      firstCallReject(error);
+    }
+  };
+
+  setTimeout(
+    () => {
+      firstCallReject(new Error('First call to function took longer than expected'));
+    },
+    timeoutMs,
+  );
+
+  return {
+    promise: firstCallPromise,
+    fn: wrapped,
+  };
+}