Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using a custom adapter breaks testing dialogs (broken since v4.8.0, works with 4.6.2) #1985

Closed
etiennellipse opened this issue Jun 23, 2020 · 9 comments
Assignees

Comments

@etiennellipse
Copy link
Contributor

We have developed a custom bot worker, which acts as a wrapper to get some information easily from within our conversations. For example, getUser(), getIncomingMessageEntity() which are specific to our solution. This bot worker is set by our custom adapter (a modified version of the Web Adapter from botkit) using the botkit_worker property.

We test our dialogs using the BotkitTestClient, of course. In those tests, we use a fake adapter and worker in order to be able to test dialogs that call the botWorker.getUser() method for example.

All was well and good up until botkit v4.6.2. Since v4.8.0, it does not work anymore.

This jest unit test works on botkit v4.6.2, but not on 4.8.0 or 4.9.0:

import {
    Botkit, BotkitConversation, BotkitTestClient, BotWorker,
} from "botkit";
import { BotkitBotFrameworkAdapter } from "botkit/lib/adapter";

class FakeBotWorker extends BotWorker {
    public getUser(): { id: number; name: string } {
        return {
            id: 123,
            name: "Roger",
        };
    }
}

class FakeAdapter extends BotkitBotFrameworkAdapter {
    // Enables overriding the type of the BotWorker
    // (this uses a Botkit features that allows setting a worker type)
    public botkit_worker = FakeBotWorker;
}

function createDialog(controller: Botkit): BotkitConversation {
    const dialog = new BotkitConversation("try_custom_worker", controller);

    dialog.ask("How you like me now?", async (response, convo, bot: FakeBotWorker) => {
        const botUser = bot.getUser();
        return bot.say(`You are: ${botUser.name}`);
    }, "question");

    return dialog;
}

describe("Test something with custom worker", () => {
    let botkit;
    let client;
    let testAdapter;

    afterAll(() => {
        botkit.shutdown();
    });

    it("bot can access user identity through custom bot worker", async () => {
        testAdapter = new FakeAdapter({});
        botkit = new Botkit({
            adapter: testAdapter,
        });
        botkit.addDialog(createDialog(botkit));
        client = new BotkitTestClient("test", botkit, ["try_custom_worker"]);

        // Test the dialog through the client
        let message = await client.sendActivity("");
        expect(message.text).toBe("How you like me now?");
        message = await client.sendActivity("nice!");
        expect(message.text).toBe("You are: Roger");
    });
});

What was the result you received?

When running this test against botkit v4.9.0, I get TypeError: bot.getUser is not a function

image

What did you expect?

It should work on Botkit latest version as it does in 4.6.2.

Investigation

In v4.6.2, botkit was using the adapter set in the controller instance in the Botkit.spawn() method (this.adapter). With v4.9.0, the testAdapter is in config.context.adapter, so it resolves to the testAdapter one instead.

Context:

  • Botkit version: 4.8.0 and 4.9.0
  • Messaging Platform: Unit Tests
  • Node version: v12.14.0
  • Os: MacOS
@etiennellipse
Copy link
Contributor Author

I was able to create a new TestClient by copying the one from Botkit completely and changing the instantiated type to my custom adapter in it. This was, the test works again.

This seems like a good solution, but I don't know if it's the correct way to fix it. I would like @benbrown opinion on that.

Sorry for the code quality here; it's a very rough proof of concept.

/* eslint-disable */
import {
    Botkit, BotkitConversation, BotWorker,
} from "botkit";
import {
    Activity,
    AutoSaveStateMiddleware,
    ConversationState,
    MemoryStorage,
    Middleware,
    TestAdapter,
    TurnContext,
} from "botbuilder-core";
import { Dialog, DialogSet, DialogTurnResult, DialogTurnStatus } from 'botbuilder-dialogs';

class FakeBotWorker extends BotWorker {
    public getUser(): { id: number; name: string } {
        return {
            id: 123,
            name: "Roger",
        };
    }
}

// Copy pasta from botkit testClient.ts, with the type of the adapter changed to mine.
class MyTestClient {
    private readonly _callback: (turnContext: TurnContext) => Promise<void>;

    private readonly _testAdapter: TestAdapter;

    public dialogTurnResult: DialogTurnResult;

    public conversationState: ConversationState;

    // eslint-disable-next-line max-len
    public constructor(channelOrAdapter: string | TestAdapter, bot: Botkit, dialogToTest: string | string[], initialDialogOptions?: any, middlewares?: Middleware[], conversationState?: ConversationState) {
        this.conversationState = conversationState || new ConversationState(new MemoryStorage());

        const dialogState = this.conversationState.createProperty("DialogState");

        let targetDialogs = [];
        if (Array.isArray(dialogToTest)) {
            dialogToTest.forEach((dialogName) => {
                targetDialogs.push(
                    bot.dialogSet.find(dialogName),
                );
                targetDialogs.push(
                    bot.dialogSet.find(`${dialogName}_default_prompt`),
                );
                targetDialogs.push(
                    bot.dialogSet.find(`${dialogName}:botkit-wrapper`),
                );
            });
            dialogToTest = dialogToTest[0];
        } else {
            targetDialogs = [
                bot.dialogSet.find(dialogToTest),
                bot.dialogSet.find(`${dialogToTest}_default_prompt`),
                bot.dialogSet.find(`${dialogToTest}:botkit-wrapper`),
            ];
        }

        this._callback = this.getDefaultCallback(targetDialogs, initialDialogOptions || null, dialogState);

        if (typeof channelOrAdapter === "string") {
            this._testAdapter = new FakeAdapter(this._callback, { channelId: channelOrAdapter }).use(new AutoSaveStateMiddleware(this.conversationState));
        } else {
            this._testAdapter = channelOrAdapter;
        }

        this.addUserMiddlewares(middlewares);
    }

    /**
     * Send an activity into the dialog.
     * @returns a TestFlow that can be used to assert replies etc
     * @param activity an activity potentially with text
     *
     * ```javascript
     * DialogTest.send('hello').assertReply('hello yourself').then(done);
     * ```
     */
    public async sendActivity(activity: Partial<Activity> | string): Promise<any> {
        await this._testAdapter.receiveActivity(activity);
        return this._testAdapter.activityBuffer.shift();
    }

    /**
     * Get the next reply waiting to be delivered (if one exists)
     */
    public getNextReply(): Partial<Activity> {
        return this._testAdapter.activityBuffer.shift();
    }

    private getDefaultCallback(targetDialogs: Dialog[], initialDialogOptions: any, dialogState: any): (turnContext) => Promise<void> {
        return async (turnContext): Promise<void> => {
            const dialogSet = new DialogSet(dialogState);
            targetDialogs.forEach((targetDialog) => dialogSet.add(targetDialog));

            const dialogContext = await dialogSet.createContext(turnContext);
            this.dialogTurnResult = await dialogContext.continueDialog();
            if (this.dialogTurnResult.status === DialogTurnStatus.empty) {
                this.dialogTurnResult = await dialogContext.beginDialog(targetDialogs[0].id, initialDialogOptions);
            }
        };
    }

    private addUserMiddlewares(middlewares: Middleware[]): void {
        if (middlewares != null) {
            middlewares.forEach((middleware) => {
                this._testAdapter.use(middleware);
            });
        }
    }
}

class FakeAdapter extends TestAdapter {
    // Enables overriding the type of the BotWorker
    // (this uses a Botkit features that allows setting a worker type)
    public botkit_worker = FakeBotWorker;
}

function createDialog(controller: Botkit): BotkitConversation {
    const dialog = new BotkitConversation("try_custom_worker", controller);

    dialog.ask("How you like me now?", async (response, convo, bot: FakeBotWorker) => {
        const botUser = bot.getUser();
        return bot.say(`You are: ${botUser.name}`);
    }, "question");

    return dialog;
}

describe("Test something with custom worker", () => {
    let botkit;
    let client;
    let testAdapter;

    afterAll(() => {
        botkit.shutdown();
    });

    it("bot can access user identity through custom bot worker", async () => {
        // eslint-disable-next-line @typescript-eslint/no-empty-function
        //testAdapter = new FakeAdapter(async () => {});
        botkit = new Botkit({
            adapter: testAdapter,
        });
        botkit.addDialog(createDialog(botkit));
        client = new MyTestClient("test", botkit, ["try_custom_worker"]);

        // Test the dialog through the client
        let message = await client.sendActivity("");
        expect(message.text).toBe("How you like me now?");
        message = await client.sendActivity("nice!");
        expect(message.text).toBe("You are: Roger");
    });
});

@stale
Copy link

stale bot commented Aug 22, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 22, 2020
@etiennellipse
Copy link
Contributor Author

Still revelant.

@stale stale bot removed the stale label Aug 24, 2020
@benbrown benbrown self-assigned this Aug 24, 2020
@benbrown
Copy link
Contributor

@etiennellipse I was able to achieve this with a slightly simpler approach -- derive a subclass of the BotkitTestClient, and override the _testAdapter member with your customized FakeAdapter (now derived from TestAdapter). See below:

class FakeAdapter extends TestAdapter {
  // Enables overriding the type of the BotWorker
  // (this uses a Botkit features that allows setting a worker type)
  botkit_worker = FakeBotWorker;
}

class CustomTestClient extends BotkitTestClient {
  constructor(channelId, bot, dialogToTest) {
    super(channelId, bot, dialogToTest);
    this._testAdapter = new FakeAdapter(this._callback, { channelId: channelId }).use(new AutoSaveStateMiddleware(this.conversationState));
  }
}

Then, use this instead of the BotkitTestClient.

@benbrown
Copy link
Contributor

I am including your new test and this example in 4.10!

@etiennellipse
Copy link
Contributor Author

@benbrown Cool, thanks! I'll give it a try very soon.

@benbrown
Copy link
Contributor

A new test based on this is included in 4.10. @etiennellipse let me know if anything remains to be done, otherwise let's close this! :)

@etiennellipse
Copy link
Contributor Author

@benbrown The solution you offer works! Thanks.

There are a few quirks to iron out for TypeScript compatibility. Since the _testAdapter and _callback properties are private readonly, they cannot be accessed/changed from outside the BotkitTestClient class itself. My suggestion is to loosen the constraints on those properties, putting them only protected. I will submit a PR immediately for this.

@benbrown
Copy link
Contributor

Thanks @etiennellipse! I will include that PR in an upcoming release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants