From 04999f678bee61af98c9666942bde9e5a843a317 Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Tue, 26 Jun 2018 16:18:31 +0100 Subject: [PATCH 01/11] Rename {MessageType => Attachment} --- src/clients/botframework_client.ts | 12 +++---- src/runner.ts | 6 ++-- src/spec/message.ts | 4 +-- src/spec/response.ts | 18 +++++------ src/test/runner.ts | 50 +++++++++++++++--------------- src/test/spec/response.ts | 14 ++++----- 6 files changed, 50 insertions(+), 54 deletions(-) diff --git a/src/clients/botframework_client.ts b/src/clients/botframework_client.ts index 041d607..4f720c8 100644 --- a/src/clients/botframework_client.ts +++ b/src/clients/botframework_client.ts @@ -6,7 +6,7 @@ import { Activity, Message as BotMessage } from 'botframework-directlinejs'; import { Client } from './client_interface'; -import { Message, MessageType } from '../spec/message'; +import { Message, Attachment } from '../spec/message'; import * as program from 'commander'; import * as chalk from 'chalk'; @@ -78,16 +78,16 @@ export class BotFrameworkClient extends Client { } const message: Message = { user, - messageTypes: [ - dlMessage.text && MessageType.Text, + attachments: [ + dlMessage.text && Attachment.Text, dlMessage.attachments && dlMessage.attachments.some(a => - a.contentType.includes('image')) && MessageType.Image, + a.contentType.includes('image')) && Attachment.Image, dlMessage.attachments && dlMessage.attachments.some(a => - a.contentType.includes('hero')) && MessageType.Card, + a.contentType.includes('hero')) && Attachment.Card, ].filter(m => m), text: dlMessage.text || null, }; - if (message.messageTypes.length) { + if (message.attachments.length) { callback(message); return; } diff --git a/src/runner.ts b/src/runner.ts index 794d053..15898ce 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -1,7 +1,7 @@ 'use strict'; import { Client } from './clients/client_interface'; import { Dialogue } from './spec/dialogue'; -import { Message, MessageType } from './spec/message'; +import { Message, Attachment } from './spec/message'; import { Response } from './spec/response'; import { Turn, TurnType } from './spec/turn'; import { Config } from './config'; @@ -156,7 +156,7 @@ export class Runner { if (response !== null) { if (program.verbose) { const timing = (new Date().getTime() - this.timing) || 0; - if (response.messageTypes.includes(MessageType.Text)) { + if (response.attachments.includes(Attachment.Text)) { const texts = response.text.split('\n'); const truncatedText = texts.length > 5 ? texts.slice(0, 5).join('\n') + '...' : response.text; @@ -261,7 +261,7 @@ export class Runner { } this.client.send({ user, - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: next.query, }); } diff --git a/src/spec/message.ts b/src/spec/message.ts index 909345b..ec4e2b6 100644 --- a/src/spec/message.ts +++ b/src/spec/message.ts @@ -1,11 +1,11 @@ -export enum MessageType { +export enum Attachment { Text = 'Text', Image = 'Image', Card = 'Card', } export interface Message { - messageTypes: MessageType[]; + attachments: Attachment[]; text: string; user: string; } diff --git a/src/spec/response.ts b/src/spec/response.ts index 7863127..36e4a3c 100644 --- a/src/spec/response.ts +++ b/src/spec/response.ts @@ -1,16 +1,14 @@ -import { Message, MessageType } from './message'; +import { Message, Attachment } from './message'; import { Translator } from '../translator'; import * as program from 'commander'; const wildcardRegex: RegExp = /<\*>/g; const wordRegex: RegExp = //g; const numberRegex: RegExp = //g; -const imageRegex: RegExp = //g; -const cardsRegex: RegExp = //g; const regexRegex: RegExp = /<\((.+?)\)>/g; export class Response { - responseType: MessageType; + attachments: Attachment[]; private textMatchChecker: RegExp; original: string; @@ -18,21 +16,21 @@ export class Response { this.original = responseData.trim(); switch (this.original) { case '': - this.responseType = MessageType.Image; + this.responseType = Attachment.Image; break; case '': - this.responseType = MessageType.Card; + this.responseType = Attachment.Card; break; default: - this.responseType = MessageType.Text; + this.responseType = Attachment.Text; this.textMatchChecker = new RegExp(Response.transformTags(this.original)); } } matches(message: Message): boolean { - if (!message.messageTypes.includes(this.responseType)) { + if (!message.attachments.includes(this.responseType)) { return false; - } else if (this.responseType === MessageType.Text) { + } else if (this.responseType === Attachment.Text) { return message.text.trim().match(this.textMatchChecker) !== null; } else { return true; @@ -43,8 +41,6 @@ export class Response { regexRegex.lastIndex = 0; const taggedText = text - .replace(imageRegex, '') - .replace(cardsRegex, '') .replace(wildcardRegex, '<([\\s\\S]*?)>') .replace(wordRegex, '<([^ ]+?)>') .replace(numberRegex, '<([0-9\.,-]+)>'); diff --git a/src/test/runner.ts b/src/test/runner.ts index 48fb928..c8295dd 100644 --- a/src/test/runner.ts +++ b/src/test/runner.ts @@ -3,7 +3,7 @@ import { Runner } from '../runner'; import { MockClient } from '../clients/mock_client'; import { defaultConfig } from '../config'; import { Dialogue } from '../spec/dialogue'; -import { MessageType } from '../spec/message'; +import { Attachment } from '../spec/message'; import * as fs from 'fs'; import * as path from 'path'; @@ -27,19 +27,19 @@ describe('runner.ts', () => { }); expect(client.read(username)).to.equal('Hello'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Hi', user: username, }); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'How are you?', user: username, }); expect(client.read(username)).to.equal('I\'m good'); expect(client.read(username)).to.equal('Yourself?'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'I\'m good too', user: username, }); @@ -66,19 +66,19 @@ describe('runner.ts', () => { }); expect(client.read(username)).to.equal('Hello'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: '123', user: username, }); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Hi', user: username, }); expect(client.read(username)).to.equal('I\'m good'); expect(client.read(username)).to.equal('Yourself?'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'I\'m good too', user: username, }); @@ -130,7 +130,7 @@ describe('runner.ts', () => { terminated: false, }); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Bye', user: username, }); @@ -174,14 +174,14 @@ describe('runner.ts', () => { expect(client.read(users[0])).to.equal('Can you assist me?'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Ok', user: users[0], }); // This ends user0 client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Howdy', user: users[1], }); @@ -189,21 +189,21 @@ describe('runner.ts', () => { expect(client.read(users[1])).to.equal('Are you a cowboy?'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Yes', user: users[1], }); expect(client.read(users[1])).to.equal('Really?'); expect(client.read(users[1])).to.equal('Can you assist me?'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'No', user: users[1], }); // This ends user1 client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Howdy', user: users[2], }); @@ -211,7 +211,7 @@ describe('runner.ts', () => { expect(client.read(users[2])).to.equal('Are you a cowboy?'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'No', user: users[2], }); @@ -251,13 +251,13 @@ describe('runner.ts', () => { expect(client.read(users[0])).to.equal('Can you assist me?'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Random text', user: users[0], }); // user0 fails now client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Howdy', user: users[1], }); @@ -266,7 +266,7 @@ describe('runner.ts', () => { // user1 shortcircuits client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Howdy', user: users[2], }); @@ -309,26 +309,26 @@ describe('runner.ts', () => { expect(client.read(username)).to.equal('Hi there'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Hey', user: username, }); expect(client.read(username)).to.equal('Hello'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Hi', user: username, }); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'How are you?', user: username, }); expect(client.read(username)).to.equal('I\'m good'); expect(client.read(username)).to.equal('Yourself?'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'I\'m good too', user: username, }); @@ -361,14 +361,14 @@ describe('runner.ts', () => { expect(client.read(username)).to.equal('Show me some cats'); client.reply({ - messageTypes: [MessageType.Image], + attachments: [Attachment.Image], text: null, user: username, }); expect(client.read(username)).to.equal('What can you do?'); client.reply({ - messageTypes: [MessageType.Card], + attachments: [Attachment.Cards], text: null, user: username, }); @@ -401,7 +401,7 @@ describe('runner.ts', () => { expect(client.read(username)).to.equal('Hey there'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'How are you?', user: username, }); @@ -412,7 +412,7 @@ describe('runner.ts', () => { setTimeout(() => { expect(client.read(username)).to.equal('Hola'); client.reply({ - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], text: 'Como estas?', user: username, }); diff --git a/src/test/spec/response.ts b/src/test/spec/response.ts index d87df03..03283b9 100644 --- a/src/test/spec/response.ts +++ b/src/test/spec/response.ts @@ -1,11 +1,11 @@ import { expect } from 'chai'; import { Response } from '../../spec/response'; -import { Message, MessageType } from '../../spec/message'; +import { Message, Attachment } from '../../spec/message'; export function createTextMessage(text: string): Message { return { text, - messageTypes: [MessageType.Text], + attachments: [Attachment.Text], user: null, }; } @@ -20,26 +20,26 @@ describe('response.ts', () => { expect(Response.transformTags('Hey there')).to.equal('^Hey ([^ ]+?) there$'); }); it('should instantiate correctly', () => { - expect(new Response(' ').responseType).to.equal(MessageType.Card); + expect(new Response(' ').responseType).to.equal(Attachment.Card); }); it('should instantiate correctly', () => { - expect(new Response(' ').responseType).to.equal(MessageType.Image); + expect(new Response(' ').responseType).to.equal(Attachment.Image); }); it('should match correctly', () => { - expect(new Response(' ').responseType).to.equal(MessageType.Card); + expect(new Response(' ').responseType).to.equal(Attachment.Card); }); it('should match correctly', () => { expect(new Response(' ').matches({ user: null, text: null, - messageTypes: [MessageType.Image], + attachments: [Attachment.Image], })).to.be.true; }); it('should match correctly', () => { expect(new Response(' ').matches({ user: null, text: null, - messageTypes: [MessageType.Card], + attachments: [Attachment.Cards], })).to.be.true; }); it('should match simple phrases correctly', () => { From b86b418d787edcc98a1eb194e5ff1c62dbf89034 Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Tue, 26 Jun 2018 16:22:42 +0100 Subject: [PATCH 02/11] Remove Text as an attachment type --- src/clients/botframework_client.ts | 14 ++++------ src/runner.ts | 4 +-- src/spec/message.ts | 10 +++++-- src/test/runner.ts | 44 +++++++++++++++--------------- src/test/spec/response.ts | 2 +- 5 files changed, 39 insertions(+), 35 deletions(-) diff --git a/src/clients/botframework_client.ts b/src/clients/botframework_client.ts index 4f720c8..817bc9a 100644 --- a/src/clients/botframework_client.ts +++ b/src/clients/botframework_client.ts @@ -78,16 +78,14 @@ export class BotFrameworkClient extends Client { } const message: Message = { user, - attachments: [ - dlMessage.text && Attachment.Text, - dlMessage.attachments && dlMessage.attachments.some(a => - a.contentType.includes('image')) && Attachment.Image, - dlMessage.attachments && dlMessage.attachments.some(a => - a.contentType.includes('hero')) && Attachment.Card, - ].filter(m => m), + attachments: dlMessage.map((a) => { + if (a.contentType.includes('image')) return Attachment.Image; + if (a.contentType.includes('hero')) return Attachment.Cards; + return Attachment.Other; + }), text: dlMessage.text || null, }; - if (message.attachments.length) { + if (message.text || message.attachments.length) { callback(message); return; } diff --git a/src/runner.ts b/src/runner.ts index 15898ce..4b724c8 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -156,7 +156,7 @@ export class Runner { if (response !== null) { if (program.verbose) { const timing = (new Date().getTime() - this.timing) || 0; - if (response.attachments.includes(Attachment.Text)) { + if (response.attachments.length === 0) { const texts = response.text.split('\n'); const truncatedText = texts.length > 5 ? texts.slice(0, 5).join('\n') + '...' : response.text; @@ -261,7 +261,7 @@ export class Runner { } this.client.send({ user, - attachments: [Attachment.Text], + attachments: [], text: next.query, }); } diff --git a/src/spec/message.ts b/src/spec/message.ts index ec4e2b6..810450c 100644 --- a/src/spec/message.ts +++ b/src/spec/message.ts @@ -1,7 +1,13 @@ +/* + * An enumeration of the possible attachment types. + * + * `Other` is a catch-all for any attachment type that is not (yet) explicitly + * supported. + */ export enum Attachment { - Text = 'Text', Image = 'Image', - Card = 'Card', + Cards = 'Cards', + Other = 'Other', } export interface Message { diff --git a/src/test/runner.ts b/src/test/runner.ts index c8295dd..a07a79a 100644 --- a/src/test/runner.ts +++ b/src/test/runner.ts @@ -27,19 +27,19 @@ describe('runner.ts', () => { }); expect(client.read(username)).to.equal('Hello'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Hi', user: username, }); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'How are you?', user: username, }); expect(client.read(username)).to.equal('I\'m good'); expect(client.read(username)).to.equal('Yourself?'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'I\'m good too', user: username, }); @@ -66,19 +66,19 @@ describe('runner.ts', () => { }); expect(client.read(username)).to.equal('Hello'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: '123', user: username, }); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Hi', user: username, }); expect(client.read(username)).to.equal('I\'m good'); expect(client.read(username)).to.equal('Yourself?'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'I\'m good too', user: username, }); @@ -130,7 +130,7 @@ describe('runner.ts', () => { terminated: false, }); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Bye', user: username, }); @@ -174,14 +174,14 @@ describe('runner.ts', () => { expect(client.read(users[0])).to.equal('Can you assist me?'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Ok', user: users[0], }); // This ends user0 client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Howdy', user: users[1], }); @@ -189,21 +189,21 @@ describe('runner.ts', () => { expect(client.read(users[1])).to.equal('Are you a cowboy?'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Yes', user: users[1], }); expect(client.read(users[1])).to.equal('Really?'); expect(client.read(users[1])).to.equal('Can you assist me?'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'No', user: users[1], }); // This ends user1 client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Howdy', user: users[2], }); @@ -211,7 +211,7 @@ describe('runner.ts', () => { expect(client.read(users[2])).to.equal('Are you a cowboy?'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'No', user: users[2], }); @@ -251,13 +251,13 @@ describe('runner.ts', () => { expect(client.read(users[0])).to.equal('Can you assist me?'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Random text', user: users[0], }); // user0 fails now client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Howdy', user: users[1], }); @@ -266,7 +266,7 @@ describe('runner.ts', () => { // user1 shortcircuits client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Howdy', user: users[2], }); @@ -309,26 +309,26 @@ describe('runner.ts', () => { expect(client.read(username)).to.equal('Hi there'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Hey', user: username, }); expect(client.read(username)).to.equal('Hello'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Hi', user: username, }); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'How are you?', user: username, }); expect(client.read(username)).to.equal('I\'m good'); expect(client.read(username)).to.equal('Yourself?'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'I\'m good too', user: username, }); @@ -401,7 +401,7 @@ describe('runner.ts', () => { expect(client.read(username)).to.equal('Hey there'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'How are you?', user: username, }); @@ -412,7 +412,7 @@ describe('runner.ts', () => { setTimeout(() => { expect(client.read(username)).to.equal('Hola'); client.reply({ - attachments: [Attachment.Text], + attachments: [], text: 'Como estas?', user: username, }); diff --git a/src/test/spec/response.ts b/src/test/spec/response.ts index 03283b9..ef45111 100644 --- a/src/test/spec/response.ts +++ b/src/test/spec/response.ts @@ -5,7 +5,7 @@ import { Message, Attachment } from '../../spec/message'; export function createTextMessage(text: string): Message { return { text, - attachments: [Attachment.Text], + attachments: [], user: null, }; } From a748ad2836385507885714835d7ce53786f01809 Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Tue, 26 Jun 2018 16:26:37 +0100 Subject: [PATCH 03/11] Update Response to handle multiple attachments --- src/clients/botframework_client.ts | 2 +- src/spec/message.ts | 9 ++-- src/spec/response.ts | 67 +++++++++++++++++++++++------- 3 files changed, 58 insertions(+), 20 deletions(-) diff --git a/src/clients/botframework_client.ts b/src/clients/botframework_client.ts index 817bc9a..5692568 100644 --- a/src/clients/botframework_client.ts +++ b/src/clients/botframework_client.ts @@ -78,7 +78,7 @@ export class BotFrameworkClient extends Client { } const message: Message = { user, - attachments: dlMessage.map((a) => { + attachments: dlMessage.attachments.map((a) => { if (a.contentType.includes('image')) return Attachment.Image; if (a.contentType.includes('hero')) return Attachment.Cards; return Attachment.Other; diff --git a/src/spec/message.ts b/src/spec/message.ts index 810450c..2523356 100644 --- a/src/spec/message.ts +++ b/src/spec/message.ts @@ -2,12 +2,13 @@ * An enumeration of the possible attachment types. * * `Other` is a catch-all for any attachment type that is not (yet) explicitly - * supported. + * supported. The enum values are used to build the regexes for the attachment + * wildcards, e.g. is derived from 'IMAGE'. */ export enum Attachment { - Image = 'Image', - Cards = 'Cards', - Other = 'Other', + Image = 'IMAGE', + Cards = 'CARDS', + Other = 'OTHER', } export interface Message { diff --git a/src/spec/response.ts b/src/spec/response.ts index 36e4a3c..f2610d4 100644 --- a/src/spec/response.ts +++ b/src/spec/response.ts @@ -7,34 +7,71 @@ const wordRegex: RegExp = //g; const numberRegex: RegExp = //g; const regexRegex: RegExp = /<\((.+?)\)>/g; +// Matches an attachment wildcard (e.g. ) at the _end_ of responseData. +// +// The matched part is just the word in the brackets ('IMAGE'), which can be +// directly compared against the Attachment enum (from which it was generated). +const attachmentAlternatives = Object.values(Attachment).join('|'); +const attachmentsRegex = new RegExp(`\\s*<(${attachmentAlternatives})>$`); + export class Response { attachments: Attachment[]; private textMatchChecker: RegExp; + private bodyText: string; original: string; constructor(responseData: string) { + this.attachments = []; this.original = responseData.trim(); - switch (this.original) { - case '': - this.responseType = Attachment.Image; - break; - case '': - this.responseType = Attachment.Card; - break; - default: - this.responseType = Attachment.Text; - this.textMatchChecker = new RegExp(Response.transformTags(this.original)); + this.bodyText = this.original; + + // Construct a list of attachments from the attachment wildcards at the + // end of the response data. The end result is that `this.bodyText` + // contains only the body text and potentially some inline wildcards; + // and `this.attachments` contains a list of attachment types, in the + // same order as they originally were in the supplied data. + let match; + while ((match = this.bodyText.match(attachmentsRegex)) != null) { + this.attachments.push(match[1]); + this.bodyText = this.bodyText.slice(0, match.index); } + this.attachments.reverse(); + + // Construct a regex to match the remaining body text in terms of its + // inline wildcards. + this.textMatchChecker = new RegExp(Response.transformTags(this.bodyText)); } matches(message: Message): boolean { - if (!message.attachments.includes(this.responseType)) { + + // If we are expecting a message body, check that we have one... + if (this.bodyText && !message.text) { + return false; + } + + // ...and check that the content is the same. + // + // Note that we ignore the message body altogether if the spec doesn't + // include one. This is for backwards compatibility with when + // and could only be used in place of the entire message. + if (this.bodyText && message.text.trim().match(this.textMatchChecker) === null) { + return false; + } + + // Check that we have the right number of attachments. + if (this.attachments.length !== message.attachments.length) { return false; - } else if (this.responseType === Attachment.Text) { - return message.text.trim().match(this.textMatchChecker) !== null; - } else { - return true; } + + // Check that we have the right types of attachments. + for (let i = 0; i < this.attachments.length; i += 1) { + if (this.attachments[i] !== message.attachments[i]) { + return false; + } + } + + // Everything appears to be in order. + return true; } static transformTags(text: string): string { From ac0dd3bec195b6a55965d7c07d795ed7a7c57972 Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Tue, 26 Jun 2018 16:53:43 +0100 Subject: [PATCH 04/11] Simplify response attachments comparison --- src/spec/response.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/spec/response.ts b/src/spec/response.ts index f2610d4..5aecdcb 100644 --- a/src/spec/response.ts +++ b/src/spec/response.ts @@ -58,18 +58,15 @@ export class Response { return false; } + // Check that we have the same number, kind, and order of attachments. + // We can use JSON.stringify here since we know attachments is just a + // simple array of strings. + // Check that we have the right number of attachments. - if (this.attachments.length !== message.attachments.length) { + if (JSON.stringify(this.attachments) !== JSON.stringify(message.attachments)) { return false; } - // Check that we have the right types of attachments. - for (let i = 0; i < this.attachments.length; i += 1) { - if (this.attachments[i] !== message.attachments[i]) { - return false; - } - } - // Everything appears to be in order. return true; } From 65f1108952b972b7e41284b1add8d000cb8ccb8f Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Tue, 26 Jun 2018 17:06:46 +0100 Subject: [PATCH 05/11] Add extra tests for backwards compatibility --- src/test/spec/response.ts | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/test/spec/response.ts b/src/test/spec/response.ts index ef45111..fcb759b 100644 --- a/src/test/spec/response.ts +++ b/src/test/spec/response.ts @@ -19,15 +19,6 @@ describe('response.ts', () => { expect(Response.transformTags('Hey <*>')).to.equal('^Hey ([\\s\\S]*?)$'); expect(Response.transformTags('Hey there')).to.equal('^Hey ([^ ]+?) there$'); }); - it('should instantiate correctly', () => { - expect(new Response(' ').responseType).to.equal(Attachment.Card); - }); - it('should instantiate correctly', () => { - expect(new Response(' ').responseType).to.equal(Attachment.Image); - }); - it('should match correctly', () => { - expect(new Response(' ').responseType).to.equal(Attachment.Card); - }); it('should match correctly', () => { expect(new Response(' ').matches({ user: null, @@ -42,6 +33,24 @@ describe('response.ts', () => { attachments: [Attachment.Cards], })).to.be.true; }); + it('should ignore text with if no text is expected', () => { + // This is required for backwards compatibility with when + // could only be used as the entire response. + expect(new Response(' ').matches({ + user: null, + text: ' Here is your image: ', + attachments: [Attachment.Image], + })).to.be.true; + }); + it('should ignore text with if no text is expected', () => { + // This is required for backwards compatibility with when + // could only be used as the entire response. + expect(new Response(' ').matches({ + user: null, + text: ' Here are your options: ', + attachments: [Attachment.Cards], + })).to.be.true; + }); it('should match simple phrases correctly', () => { expect(new Response('Hello there!').matches(createTextMessage(' Hello there! '))) .to.be.true; From dd834c8834d1de565b4293b96548ef58a196518f Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Tue, 26 Jun 2018 17:22:08 +0100 Subject: [PATCH 06/11] Add tests for the new functionality --- src/test/spec/response.ts | 56 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/src/test/spec/response.ts b/src/test/spec/response.ts index fcb759b..d5f3f93 100644 --- a/src/test/spec/response.ts +++ b/src/test/spec/response.ts @@ -51,6 +51,62 @@ describe('response.ts', () => { attachments: [Attachment.Cards], })).to.be.true; }); + it('should match correct text with an attachment if text is expected', () => { + expect(new Response('Here are your options: ').matches({ + user: null, + text: ' Here are your options: ', + attachments: [Attachment.Cards], + })).to.be.true; + }); + it('should not match incorrect text with an attachment if text is expected', () => { + expect(new Response('Here are your options: ').matches({ + user: null, + text: 'choose from ', + attachments: [Attachment.Cards], + })).to.be.false; + }); + it('should not match incorrect type of attachment', () => { + expect(new Response('').matches({ + user: null, + text: null, + attachments: [Attachment.Other], + })).to.be.false; + }); + it('should not match a missing attachment', () => { + expect(new Response('Here you go: ').matches({ + user: null, + text: 'Here you go:', + attachments: [], + })).to.be.false; + }); + it('should match multiple attachments', () => { + expect(new Response('Here you go: ').matches({ + user: null, + text: 'Here you go:', + attachments: [Attachment.Image, Attachment.Image], + })).to.be.true; + }); + it('should not match the wrong number of attachments', () => { + expect(new Response('Here you go: ').matches({ + user: null, + text: 'Here you go:', + attachments: [Attachment.Image, Attachment.Image, Attachment.Image], + })).to.be.false; + }); + it('should match different types of attachments', () => { + expect(new Response(' Here you go: ').matches({ + user: null, + text: 'Here you go:', + attachments: [Attachment.Image, Attachment.Cards], + })).to.be.true; + }); + it('should not match the wrong order of attachments', () => { + expect(new Response(' ').matches({ + user: null, + text: null, + attachments: [Attachment.Cards, Attachment.Image], + })).to.be.false; + }); it('should match simple phrases correctly', () => { expect(new Response('Hello there!').matches(createTextMessage(' Hello there! '))) .to.be.true; From 8c6ea46bd606bbecf87c6174140bbdc06009dbea Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Tue, 26 Jun 2018 17:46:37 +0100 Subject: [PATCH 07/11] Update documentation with new attachment features --- README.md | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index ccfce9e..795999b 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ concise, simple test files with strong flexibility and extensibility. - Parallel execution of tests - Test against multiple acceptable answers - Wildcard matching (``, ``, `<*>`) -- Support for testing rich attachments (``, ``) +- Support for testing rich attachments (``, ``, ``) - Support for regular expressions (`<(st|nt|nd)>`) - Show diffs on error - Conversation branches @@ -71,19 +71,39 @@ Dialogue: - Bot: ``` +Handling multiple attachments + +```YAML +Title: Conversation with rich messages +Dialogue: + - Bot: Hi + - Human: Hello + - Human: How are we doing this month? + - Bot: "Month-to-date: " + - Human: Show me ad spend compared to last month + - Bot: +``` + ## Special Tags -You can use the following tags in your test dialogue files: +You can use the following tags anywhere in the bot’s response: Tag | Meaning --- | --- `<*>` | Matches anything, including whitespaces `` | A single word without whitespaces -`` | An image attachment -`` | A card attachment `<(REGEX)>` | Any regex expression, i.e. `<([0-9]{2})>` `<$VARNAME>` | An expression from the locale/translation file +You can match one or more attachments **on their own, or at the end** of the +response: + +Tag | Meaning +--- | --- +`` | Any card attachment +`` | Any image attachment +`` | Any other type of attachment + ## Install To install from npm From ed52f83ec6dbb40ccc9754b2392634b69f4d5028 Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Wed, 27 Jun 2018 16:31:30 +0100 Subject: [PATCH 08/11] Allow attachments to come anywhere in message text --- README.md | 23 +++++++++------- src/spec/response.ts | 56 +++++++++++++++++++-------------------- src/test/spec/response.ts | 33 +++++++++++++++++------ 3 files changed, 66 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 795999b..10df35e 100644 --- a/README.md +++ b/README.md @@ -82,28 +82,31 @@ Dialogue: - Bot: "Month-to-date: " - Human: Show me ad spend compared to last month - Bot: + - Human: How is the campaign doing overall? + - Bot: According to and , it's going great! ``` +Currently, the order and type of the attachments are checked, as is the text +that surrounds them, but not the position of the attachments within the text. + +> **NOTE**: In versions `0.0.11` and earlier, the text was not checked at all +> when an attachment was present. If you were sending text along with the +> attachment, you should add it to your dialogue when upgrading. + ## Special Tags -You can use the following tags anywhere in the bot’s response: +You can use the following tags in your test dialogue files: Tag | Meaning --- | --- `<*>` | Matches anything, including whitespaces `` | A single word without whitespaces +`` | An image attachment +`` | A card attachment (this includes buttons) +`` | Any other type of attachment (video, etc.) `<(REGEX)>` | Any regex expression, i.e. `<([0-9]{2})>` `<$VARNAME>` | An expression from the locale/translation file -You can match one or more attachments **on their own, or at the end** of the -response: - -Tag | Meaning ---- | --- -`` | Any card attachment -`` | Any image attachment -`` | Any other type of attachment - ## Install To install from npm diff --git a/src/spec/response.ts b/src/spec/response.ts index 5aecdcb..13b602d 100644 --- a/src/spec/response.ts +++ b/src/spec/response.ts @@ -7,12 +7,9 @@ const wordRegex: RegExp = //g; const numberRegex: RegExp = //g; const regexRegex: RegExp = /<\((.+?)\)>/g; -// Matches an attachment wildcard (e.g. ) at the _end_ of responseData. -// -// The matched part is just the word in the brackets ('IMAGE'), which can be -// directly compared against the Attachment enum (from which it was generated). +// Use the Attachment enum values to create a regex for any attachment wildcard. const attachmentAlternatives = Object.values(Attachment).join('|'); -const attachmentsRegex = new RegExp(`\\s*<(${attachmentAlternatives})>$`); +const attachmentsRegex = new RegExp(`<(?:${attachmentAlternatives})>`, 'g'); export class Response { attachments: Attachment[]; @@ -21,39 +18,42 @@ export class Response { original: string; constructor(responseData: string) { - this.attachments = []; this.original = responseData.trim(); - this.bodyText = this.original; - // Construct a list of attachments from the attachment wildcards at the - // end of the response data. The end result is that `this.bodyText` - // contains only the body text and potentially some inline wildcards; - // and `this.attachments` contains a list of attachment types, in the - // same order as they originally were in the supplied data. - let match; - while ((match = this.bodyText.match(attachmentsRegex)) != null) { - this.attachments.push(match[1]); - this.bodyText = this.bodyText.slice(0, match.index); - } - this.attachments.reverse(); - - // Construct a regex to match the remaining body text in terms of its - // inline wildcards. + // Get a list of the Attachments in the response data. The map removes the + // angle brackets so that the resulting elements are valid enum values. + this.attachments = (this.original.match(attachmentsRegex) || []) + .map(match => match.slice(1, -1)); + + // Get what's left of the body text once the attachment wildcards have been + // removed. Each attachment consumes the surrounding whitespace, which in + // the end we replace with a single space. This is done so that you can + // write e.g. ' ' as a message spec and _not_ have the + // matcher try to match the message text with the space in between the + // wildcards in the spec. + // + // TODO: Use the array just after the map() to create an _array of match + // checkers_ so that we can validate the interleaving of attachments and + // message text, rather than ignoring where the attachments come within + // the text. + this.bodyText = this.original // 'Here you go: and ' + .split(attachmentsRegex) // ['Here you go: ', ' and ', ''] + .map(s => s.trim()) // ['Here you go:', 'and', ''] + .filter(s => s) // ['Here you go:', 'and'] + .join(' '); // 'Here you go: and' + + // Construct a regex to match the remaining body text. this.textMatchChecker = new RegExp(Response.transformTags(this.bodyText)); } matches(message: Message): boolean { - // If we are expecting a message body, check that we have one... - if (this.bodyText && !message.text) { + // Check that the message body is present or absent, as expected. + if ((this.bodyText && !message.text) || (message.text && !this.bodyText)) { return false; } - // ...and check that the content is the same. - // - // Note that we ignore the message body altogether if the spec doesn't - // include one. This is for backwards compatibility with when - // and could only be used in place of the entire message. + // If there is a message body, check that its content is correct. if (this.bodyText && message.text.trim().match(this.textMatchChecker) === null) { return false; } diff --git a/src/test/spec/response.ts b/src/test/spec/response.ts index d5f3f93..2f43037 100644 --- a/src/test/spec/response.ts +++ b/src/test/spec/response.ts @@ -33,23 +33,19 @@ describe('response.ts', () => { attachments: [Attachment.Cards], })).to.be.true; }); - it('should ignore text with if no text is expected', () => { - // This is required for backwards compatibility with when - // could only be used as the entire response. + it('should not match text with if no text is expected', () => { expect(new Response(' ').matches({ user: null, text: ' Here is your image: ', attachments: [Attachment.Image], - })).to.be.true; + })).to.be.false; }); - it('should ignore text with if no text is expected', () => { - // This is required for backwards compatibility with when - // could only be used as the entire response. + it('should not match text with if no text is expected', () => { expect(new Response(' ').matches({ user: null, text: ' Here are your options: ', attachments: [Attachment.Cards], - })).to.be.true; + })).to.be.false; }); it('should match correct text with an attachment if text is expected', () => { expect(new Response('Here are your options: ').matches({ @@ -65,6 +61,13 @@ describe('response.ts', () => { attachments: [Attachment.Cards], })).to.be.false; }); + it('should not match attachment without text if text is expected', () => { + expect(new Response('Here are your options: ').matches({ + user: null, + text: null, + attachments: [Attachment.Cards], + })).to.be.false; + }); it('should not match incorrect type of attachment', () => { expect(new Response('').matches({ user: null, @@ -107,6 +110,20 @@ describe('response.ts', () => { attachments: [Attachment.Cards, Attachment.Image], })).to.be.false; }); + it('should match multiple attachments within correct text', () => { + expect(new Response(' Compare with ').matches({ + user: null, + text: 'Compare with', + attachments: [Attachment.Image, Attachment.Other], + })).to.be.true; + }); + it('should not match multiple attachments within incorrect text', () => { + expect(new Response(' vs ').matches({ + user: null, + text: 'Compare with', + attachments: [Attachment.Image, Attachment.Other], + })).to.be.false; + }); it('should match simple phrases correctly', () => { expect(new Response('Hello there!').matches(createTextMessage(' Hello there! '))) .to.be.true; From 310ac1444b6ca025cc4f0eb5ec06ce2d72acc00a Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Wed, 27 Jun 2018 16:32:23 +0100 Subject: [PATCH 09/11] Revert "Simplify response attachments comparison" to avoid quirky stringification This reverts commit ac0dd3bec195b6a55965d7c07d795ed7a7c57972. --- src/spec/response.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/spec/response.ts b/src/spec/response.ts index 13b602d..025ba76 100644 --- a/src/spec/response.ts +++ b/src/spec/response.ts @@ -58,15 +58,18 @@ export class Response { return false; } - // Check that we have the same number, kind, and order of attachments. - // We can use JSON.stringify here since we know attachments is just a - // simple array of strings. - // Check that we have the right number of attachments. - if (JSON.stringify(this.attachments) !== JSON.stringify(message.attachments)) { + if (this.attachments.length !== message.attachments.length) { return false; } + // Check that we have the right types of attachments. + for (let i = 0; i < this.attachments.length; i += 1) { + if (this.attachments[i] !== message.attachments[i]) { + return false; + } + } + // Everything appears to be in order. return true; } From 09c3978cbf6fdf92d0879ec1a2e3f5ff6045b79b Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Wed, 27 Jun 2018 16:58:37 +0100 Subject: [PATCH 10/11] Improve verbose output of attachments --- src/runner.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/runner.ts b/src/runner.ts index 4b724c8..a33357d 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -156,12 +156,12 @@ export class Runner { if (response !== null) { if (program.verbose) { const timing = (new Date().getTime() - this.timing) || 0; - if (response.attachments.length === 0) { + if (response.text) { const texts = response.text.split('\n'); const truncatedText = texts.length > 5 ? texts.slice(0, 5).join('\n') + '...' : response.text; console.log(chalk.blue('\tBOT', response.user, ':', - truncatedText), + truncatedText, ...response.attachments.map(a => `<${a}>`)), chalk.magenta(` (${timing}ms)`)); } else { console.log(chalk.blue('\tBOT:', response.user, ':', JSON.stringify(response)), From 9c34c6d64a7b71b8cddcf803537c9a459d45ab65 Mon Sep 17 00:00:00 2001 From: Joe Cridge Date: Wed, 27 Jun 2018 17:36:47 +0100 Subject: [PATCH 11/11] Improve output when an attachment mismatch occurs --- dialogues/examples/multiple-attachments.yml | 6 ++ dialogues/examples/simple-attachment.yml | 6 ++ src/runner.ts | 5 +- src/test/runner.ts | 84 ++++++++++++++++++++- 4 files changed, 98 insertions(+), 3 deletions(-) create mode 100644 dialogues/examples/multiple-attachments.yml create mode 100644 dialogues/examples/simple-attachment.yml diff --git a/dialogues/examples/multiple-attachments.yml b/dialogues/examples/multiple-attachments.yml new file mode 100644 index 0000000..ac75dd7 --- /dev/null +++ b/dialogues/examples/multiple-attachments.yml @@ -0,0 +1,6 @@ +Title: Multiple attachments +Dialogue: + - Human: Hello + - Bot: Hi + - Human: How is my campaign doing? + - Bot: "Here are the stats for this week: " diff --git a/dialogues/examples/simple-attachment.yml b/dialogues/examples/simple-attachment.yml new file mode 100644 index 0000000..95965c5 --- /dev/null +++ b/dialogues/examples/simple-attachment.yml @@ -0,0 +1,6 @@ +Title: Simple attachment +Dialogue: + - Human: Hello + - Bot: Hi + - Human: How is my campaign doing? + - Bot: diff --git a/src/runner.ts b/src/runner.ts index a33357d..3b4ef54 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -192,12 +192,15 @@ export class Runner { expected = `\t\t${matchArray[0]}`; } + let actual = response.text ? [response.text] : []; + actual = actual.concat(response.attachments.map(a => `<${a}>`)); + this.results.set(test.dialogue, { dialogue: test.dialogue, passed: false, errorMessage: chalk.red(`\t${Runner.getUsername(test)}\n`) + `\tExpected:\n${expected}` + - `\n\tGot:\n\t\t${response.text}`, + `\n\tGot:\n\t\t${actual.join(' ') || null}`, }); this.terminateInstance(test); return; diff --git a/src/test/runner.ts b/src/test/runner.ts index a07a79a..3b0d1d2 100644 --- a/src/test/runner.ts +++ b/src/test/runner.ts @@ -97,11 +97,15 @@ describe('runner.ts', () => { client, getDialoguePath('examples/'), Object.assign({}, defaultConfig, { timeout: 5 })); - expect(runner.dialogues).to.have.length(2); + expect(runner.dialogues).to.have.length(4); expect(runner.dialogues).to.deep.include( new Dialogue(getDialoguePath('examples/simple.yml')), ).and.to.deep.include( new Dialogue(getDialoguePath('examples/branching.yml')), + ).and.to.deep.include( + new Dialogue(getDialoguePath('examples/simple-attachment.yml')), + ).and.to.deep.include( + new Dialogue(getDialoguePath('examples/multiple-attachments.yml')), ); }); @@ -118,7 +122,7 @@ describe('runner.ts', () => { }); }); - it('should fail a simple conversation if not matching', () => { + it('should fail a simple conversation if the text doesn\'t match', () => { const client = new MockClient(); const runner = new Runner(client, getDialoguePath('examples/simple.yml'), defaultConfig); setTimeout( @@ -149,6 +153,82 @@ describe('runner.ts', () => { }); }); + it('should fail a simple conversation if the attachment doesn\'t match', () => { + const client = new MockClient(); + const runner = new Runner(client, getDialoguePath('examples/simple-attachment.yml'), + defaultConfig); + setTimeout( + () => { + const username = Runner.getUsername({ + branchNumber: 0, + dialogue: new Dialogue(getDialoguePath('examples/simple-attachment.yml')), + lastMessage: 0, + terminated: false, + }); + + expect(client.read(username)).to.equal('Hello'); + client.reply({ + attachments: [], + text: 'Hi', + user: username, + }); + + expect(client.read(username)).to.equal('How is my campaign doing?'); + client.reply({ + attachments: [Attachment.Cards], + text: null, + user: username, + }); + }, + 0); + + return runner.start().then((results) => { + const testResult = results[0]; + expect(testResult.passed).to.be.false; + expect(testResult.errorMessage) + .to.contain('Expected:\n\t\t') + .and.to.contain('Got:\n\t\t'); + }); + }); + + it('should fail a conversation if attachment order doesn\'t match', () => { + const client = new MockClient(); + const runner = new Runner(client, getDialoguePath('examples/multiple-attachments.yml'), + defaultConfig); + setTimeout( + () => { + const username = Runner.getUsername({ + branchNumber: 0, + dialogue: new Dialogue(getDialoguePath('examples/multiple-attachments.yml')), + lastMessage: 0, + terminated: false, + }); + + expect(client.read(username)).to.equal('Hello'); + client.reply({ + attachments: [], + text: 'Hi', + user: username, + }); + + expect(client.read(username)).to.equal('How is my campaign doing?'); + client.reply({ + attachments: [Attachment.Cards, Attachment.Image], + text: 'Here are the stats for this week:', + user: username, + }); + }, + 0); + + return runner.start().then((results) => { + const testResult = results[0]; + expect(testResult.passed).to.be.false; + expect(testResult.errorMessage) + .to.contain('Expected:\n\t\tHere are the stats for this week: ') + .and.to.contain('Got:\n\t\tHere are the stats for this week: '); + }); + }); + it('should handle branching conversations correctly', () => { const client = new MockClient(); const runner = new Runner(client, getDialoguePath('examples/branching.yml'), defaultConfig);