Skip to content

Commit

Permalink
Remove PersistentTimer's dependency on DiscordInterface (#170)
Browse files Browse the repository at this point in the history 
* Remove PersistentTimer's dependency on DiscordInterface
* Replace 'link seams' with dependency injection
* Fix tests
#161
  • Loading branch information
@kevinlul
kevinlul authored Feb 16, 2021
1 parent 4762be9 commit 9cd9a97
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 121 deletions.
25 changes: 10 additions & 15 deletions src/TournamentManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ export interface TournamentInterface {
type Public<T> = Pick<T, keyof T>;
type Tail<T extends unknown[]> = T extends [unknown, ...infer R] ? R : never;

/// "Link seam" to mock for testing
interface PersistentTimerDelegate {
create: (...args: Tail<Parameters<typeof PersistentTimer.create>>) => ReturnType<typeof PersistentTimer.create>;
loadAll: () => ReturnType<typeof PersistentTimer.loadAll>;
}

export class TournamentManager implements TournamentInterface {
private matchScores: Record<number, MatchScore> = {};
private timers: Record<string, PersistentTimer[]> = {}; // index: tournament id
Expand All @@ -58,26 +64,15 @@ export class TournamentManager implements TournamentInterface {
private database: Public<DatabaseWrapperPostgres>,
private website: WebsiteInterface,
private templater: Templater,
private participantRole: ParticipantRoleProvider
private participantRole: ParticipantRoleProvider,
private timer: PersistentTimerDelegate
) {}

/// Link seam to override for testing
protected async createPersistentTimer(
...args: Tail<Parameters<typeof PersistentTimer.create>>
): ReturnType<typeof PersistentTimer.create> {
return await PersistentTimer.create(this.discord, ...args);
}

/// Link seam to override for testing
protected async loadPersistentTimers(): ReturnType<typeof PersistentTimer.loadAll> {
return await PersistentTimer.loadAll(this.discord);
}

public async loadTimers(): Promise<void> {
if (Object.keys(this.timers).length) {
logger.warn(new Error("loadTimers called multiple times"));
} else {
const timers = await this.loadPersistentTimers();
const timers = await this.timer.loadAll();
let count = 0;
for (const timer of timers) {
if (timer.tournament) {
Expand Down Expand Up @@ -466,7 +461,7 @@ export class TournamentManager implements TournamentInterface {
this.timers[tournament.id] = await Promise.all(
tournament.publicChannels.map(async channelId => {
await this.sendNewRoundMessage(channelId, tournament, url, round);
return await this.createPersistentTimer(
return await this.timer.create(
new Date(Date.now() + 50 * 60 * 1000), // 50 minutes
channelId,
`That's time in the round, ${role}! Please end the current phase, then the player with the lower LP must forfeit!`,
Expand Down
10 changes: 9 additions & 1 deletion src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { registerEvents } from "./events";
import { OrganiserRoleProvider } from "./role/organiser";
import { ParticipantRoleProvider } from "./role/participant";
import { Templater } from "./templates";
import { PersistentTimer, PersistentTimerDiscordDelegate } from "./timer";
import { TournamentManager } from "./TournamentManager";
import { getLogger } from "./util/logger";
import { WebsiteWrapperChallonge } from "./website/challonge";
Expand All @@ -35,7 +36,14 @@ const logger = getLogger("index");
const discord = new DiscordInterface(eris);
const organiserRole = new OrganiserRoleProvider(toRole, 0x3498db);
const participantRole = new ParticipantRoleProvider(bot, 0xe67e22);
const tournamentManager = new TournamentManager(discord, database, website, templater, participantRole);
const delegate: PersistentTimerDiscordDelegate = {
sendMessage: async (...args) => (await bot.createMessage(...args)).id,
editMessage: async (...args) => void (await bot.editMessage(...args))
};
const tournamentManager = new TournamentManager(discord, database, website, templater, participantRole, {
create: async (...args) => await PersistentTimer.create(delegate, ...args),
loadAll: async () => await PersistentTimer.loadAll(delegate)
});
registerEvents(bot, prefix, { discord, tournamentManager, organiserRole }, database, website);
discord.onDelete(msg => tournamentManager.cleanRegistration(msg));

Expand Down
50 changes: 35 additions & 15 deletions src/timer.ts
View file
Original file line number Diff line number Diff line change
@@ -1,30 +1,41 @@
import { getConnection } from "typeorm";
import { Countdown } from "./database/orm";
import { DiscordInterface } from "./discord/interface";
import { getLogger } from "./util/logger";

const logger = getLogger("timer");

export interface PersistentTimerDiscordDelegate {
/**
* Sends message to the channel specified by channelId and returns the snowflake
* for the created message. May throw exceptions.
*/
sendMessage: (channelId: string, message: string) => string | Promise<string>;
/**
* Edits the specified message, replacing it with the newMessage. May throw exceptions.
*/
editMessage: (channelId: string, messageId: string, newMessage: string) => void | Promise<void>;
}

/**
* Because this timer is not considered ready until the corresponding Discord
* message is sent and the backing database entity is serialized, the constructor
* is not public and only accessible through a static async create function.
*
* States of the object:
* init: constructor called and the timer is not installed, but the entity is not serialized
* init: constructor called and the timer is installed, but the entity is not serialized
* ready: after create returns, everything is ready for use, and isActive()
* done: either the timer ran out or was aborted, so do not use the serialized entity
*/
export class PersistentTimer {
protected interval?: NodeJS.Timeout;

/// This constructor has side effects as it immediately starts the timer!
protected constructor(protected discord: DiscordInterface, protected entity: Countdown) {
protected constructor(protected entity: Countdown, protected discord: PersistentTimerDiscordDelegate) {
this.interval = setInterval(() => this.tick(), 1000);
}

public static async create(
discord: DiscordInterface,
discord: PersistentTimerDiscordDelegate,
end: Date,
channelId: string,
finalMessage: string,
Expand All @@ -35,17 +46,17 @@ export class PersistentTimer {
const endMilli = end.getTime();
const nowMilli = Date.now();
const left = this.formatTime(endMilli - nowMilli);
const message = await discord.sendMessage(channelId, `Time left in the round: \`${left}\``);
const messageId = await discord.sendMessage(channelId, `Time left in the round: \`${left}\``);

const entity = new Countdown();
entity.end = end;
entity.channelId = channelId;
entity.messageId = message.id;
entity.messageId = messageId;
entity.finalMessage = finalMessage;
entity.cronIntervalSeconds = cronIntervalSeconds;
entity.tournamentId = tournamentId;

const timer = new PersistentTimer(discord, entity);
const timer = new PersistentTimer(entity, discord);

try {
await entity.save();
Expand All @@ -57,13 +68,13 @@ export class PersistentTimer {
return timer;
}

public static async loadAll(discord: DiscordInterface): Promise<PersistentTimer[]> {
public static async loadAll(discord: PersistentTimerDiscordDelegate): Promise<PersistentTimer[]> {
const entities = await Countdown.find();
const nowMilli = Date.now();
// Replace with for-of if too inefficient
const active = entities
.filter(entity => entity.end.getTime() > nowMilli)
.map(entity => new PersistentTimer(discord, entity));
.map(entity => new PersistentTimer(entity, discord));

// Prune expired timers after initializing the active ones
try {
Expand Down Expand Up @@ -107,17 +118,26 @@ export class PersistentTimer {
protected async tick(): Promise<void> {
const now = new Date();
if (this.entity.end <= now) {
await this.discord.sendMessage(this.entity.channelId, this.entity.finalMessage);
try {
await this.discord.sendMessage(this.entity.channelId, this.entity.finalMessage);
} catch (error) {
logger.warn(error);
}
await this.abort();
}
const secondsRemaining = Math.ceil((now.getTime() - this.entity.end.getTime()) / 1000);
if (secondsRemaining % this.entity.cronIntervalSeconds == 0) {
const left = PersistentTimer.formatTime(this.entity.end.getTime() - Date.now());
const message = await this.discord.getMessage(this.entity.channelId, this.entity.messageId);
if (message) {
message.edit(`Time left in the round: \`${left}\``);
} else {
logger.warn(`${this.entity.channelId} ${this.entity.messageId} was removed`);
try {
await this.discord.editMessage(
this.entity.channelId,
this.entity.messageId,
`Time left in the round: \`${left}\``
);
} catch (error) {
// Most likely culprit: the message was removed
logger.warn(`tick: could not edit ${this.entity.channelId} ${this.entity.messageId}`);
logger.warn(error);
}
}
}
Expand Down
71 changes: 21 additions & 50 deletions test/timer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,18 @@ import proxyquire from "proxyquire";
import sinon, { SinonSandbox } from "sinon";
import sinonChai from "sinon-chai";
import sinonTest from "sinon-test";
import { DiscordInterface } from "../src/discord/interface";
import { DiscordWrapperMock } from "./mocks/discord";

chai.use(sinonChai);
const test = sinonTest(sinon);
proxyquire.noPreserveCache();

const discordMock = new DiscordWrapperMock();
const discord = new DiscordInterface(discordMock);

// The external dependencies that we specifically mock out.
const send = sinon.stub().resolves("testId");
const edit = sinon.spy();
const discordDelegateMock = {
sendMessage: send,
editMessage: edit
};
const countdownEntityStub = {
save: sinon.stub(),
remove: sinon.stub()
Expand All @@ -21,103 +23,72 @@ const Countdown = sinon.stub().returns(countdownEntityStub);
const { PersistentTimer } = proxyquire("../src/timer", {
"./database/orm": { Countdown }
});
// The strong disadvantage of the proxyquire approach is that we lose all type-safety in tests.
// Therefore I would not recommend it for other tests, especially if they get more complex than this.

describe("PersistentTimer is a timer", function () {
// Ensures that each test does not affect the other when they use the same mocks declared above.
beforeEach(function () {
sinon.resetHistory();
});

it(
"initializes correctly",
test(async function (this: SinonSandbox) {
const sendMessageSpy = this.spy(discord, "sendMessage"); // TODO: replace when we mock Discord the same way
const timer = await PersistentTimer.create(
discord,
discordDelegateMock,
new Date(Date.now() + 10000),
"1234567890",
"Test timer done",
5
);
expect(timer.isActive()).to.be.true;
expect(sendMessageSpy).to.have.been.calledWith("1234567890", "Time left in the round: `00:10`");
expect(send).to.have.been.calledWith("1234567890", "Time left in the round: `00:10`");
expect(Countdown).to.have.been.called;
expect(countdownEntityStub.save).to.have.been.called;
expect(sendMessageSpy).to.have.been.calledBefore(countdownEntityStub.save);
expect(send).to.have.been.calledBefore(countdownEntityStub.save);
})
);

it(
"calls tick and finishes",
test(async function (this: SinonSandbox) {
const sendMessageSpy = this.spy(discord, "sendMessage"); // TODO: replace when we mock Discord the same way
const edit = this.stub();
const getMessageStub = this.stub(discord, "getMessage").returns(
Promise.resolve({
id: "",
content: "",
attachments: [],
author: "",
channelId: "",
serverId: "",
reply: this.stub(),
react: this.stub(),
edit
})
);
const timer = await PersistentTimer.create(
discord,
discordDelegateMock,
new Date(Date.now() + 15000),
"1234567890",
"Test timer done",
5
);
void timer;
// Tick should be every second
await this.clock.tickAsync(500);
expect(edit).to.not.have.been.called;
// First tick
await this.clock.tickAsync(4500);
expect(edit).to.have.been.calledOnceWith("Time left in the round: `00:10`");
expect(edit).to.have.been.calledOnceWith("1234567890", "testId", "Time left in the round: `00:10`");
// Second tick
await this.clock.tickAsync(5000);
expect(edit).to.have.been.calledTwice;
expect(edit).to.have.been.calledWith("Time left in the round: `00:05`");
expect(edit).to.have.been.calledWith("1234567890", "testId", "Time left in the round: `00:05`");
// Third tick, we should be finished
await this.clock.tickAsync(5000);
expect(edit).to.have.been.calledThrice;
expect(edit).to.have.been.calledWith("Time left in the round: `00:00`");
expect(sendMessageSpy).to.have.been.calledWith("1234567890", "Test timer done");
expect(edit).to.have.been.calledWith("1234567890", "testId", "Time left in the round: `00:00`");
expect(send).to.have.been.calledWith("1234567890", "Test timer done");
expect(countdownEntityStub.remove).to.have.been.called;
// Nothing else should happen now
await this.clock.runAllAsync();
expect(edit).to.have.been.calledThrice;
expect(countdownEntityStub.remove).to.have.been.calledOnce.and.calledOn(countdownEntityStub);
// This message id is from the "mock" DiscordWrapper. We would use a stub otherwise.
expect(getMessageStub).to.have.been.calledWithExactly("1234567890", "testId");
expect(timer.isActive()).to.be.false;
})
);

it(
"can be aborted",
test(async function (this: SinonSandbox) {
const sendMessageSpy = this.spy(discord, "sendMessage"); // TODO: replace when we mock Discord the same way
const edit = this.stub();
const getMessageStub = this.stub(discord, "getMessage").returns(
Promise.resolve({
id: "",
content: "",
attachments: [],
author: "",
channelId: "",
serverId: "",
reply: this.stub(),
react: this.stub(),
edit
})
);
void getMessageStub;
const timer = await PersistentTimer.create(
discord,
discordDelegateMock,
new Date(Date.now() + 3000),
"1234567890",
"Test timer done",
Expand All @@ -133,7 +104,7 @@ describe("PersistentTimer is a timer", function () {
await this.clock.runAllAsync();
expect(edit).to.have.been.calledOnce;
// No final message sent
expect(sendMessageSpy).to.have.been.calledOnce;
expect(send).to.have.been.calledOnce;
})
);
});
Expand Down
Loading

0 comments on commit 9cd9a97

Please sign in to comment.