Application context
I am working on a game I call "Apes and Snakes". So far the code only allows players to connect to a host using a room code and WebRTC. Players and the host send messages back and forth to each other over WebRTC. When a player receives a message, it is passed to a "handler". This kind of pattern is somewhat comparable to the way an HTTP request to a certain route gets passed on to a method of a controller in an MVC architecture.
The full repository can be found at https://github.com/colesam/apes-and-snakes, but I will be posting the relevant code snippets below. It's a react application but the portion I'm dealing with for this post mainly has to do regular javascript/typescript logic and jest, as well as reading/writing from a zustand (simplified redux) flux storage.
What I want reviewed
The code I want reviewed is the "handler" for when the host receives a reconnect message. Specifically I would like my strategy for testing this function reviewed.
The function I am testing is called handleReconnect
. What this function does in short is allow players to reconnect to the game from a new peerId if they still have their old secret key (what I use to auth/link a player to their data). My original implementation of the method looks like this:
import { TActionHandlerProps } from "../handleAction";
import GeneralError from "../../error/GeneralError";
import { getPrivate } from "../../store/privateStore";
import { StoreAction } from "../../store/StoreAction";
const handleReconnect = ({
peerId,
payload,
respond,
error,
}: TActionHandlerProps) => {
// SIDE EFFECT 1: Pull secret key map from flux storage
const { secretKeyPlayerIdMap } = getPrivate();
const playerId = secretKeyPlayerIdMap.get(payload.secretKey);
if (!playerId) {
return error(
new GeneralError("Could not find playerId. Failed to reconnect.")
);
}
// SIDE EFFECT 2: Update the peerId of the player in flux storage
StoreAction.setPlayerConnection(playerId, { peerId });
return respond();
};
export default handleReconnect;
This function has two side-effects that make this difficult to test:
getPrivate
is a function that returns the current state of my flux storage.StoreAction.setPlayerConnection
is a function that updates my store. TheStoreAction
object is really just used as a namespace for functions that mutate the store because a lot of the actions have very general names that collide with other areas of my application.
To get around this, I decided to make a factory function that returns the handler, injecting the necessary side-effects. The default export still exports the function as it exists in the previous code snippet, while exposing a way for me to inject mocks when testing.
Here is how I rewrote handleReconnect
:
import { TActionHandlerProps } from "../handleAction";
import GeneralError from "../../error/GeneralError";
import { getPrivate } from "../../store/privateStore";
import { StoreAction } from "../../store/StoreAction";
export const makeHandleReconnect = (
_getPrivate: typeof getPrivate,
_StoreAction: typeof StoreAction
) => ({ peerId, payload, respond, error }: TActionHandlerProps) => {
const { secretKeyPlayerIdMap } = _getPrivate();
const playerId = secretKeyPlayerIdMap.get(payload.secretKey);
if (!playerId) {
return error(
new GeneralError("Could not find playerId. Failed to reconnect.")
);
}
_StoreAction.setPlayerConnection(playerId, { peerId });
return respond();
};
const handleReconnect = makeHandleReconnect(getPrivate, StoreAction);
export default handleReconnect;
Here is how I tested this rewritten version:
/* eslint-disable */
import { makeHandleReconnect } from "./handleReconnect";
import { Map } from "immutable";
const makeParams = (opts = {}) => ({
peerId: "123",
payload: { secretKey: "existing-secret" },
respond: jest.fn(),
error: jest.fn(),
...opts,
});
const makeGetPrivate = (opts = {}) => () => ({
secretKeyPlayerIdMap: Map([["existing-secret", "player-id"]]),
...opts,
});
const makeStoreAction = (opts = {}) => ({
setPlayerConnection: jest.fn(),
...opts,
});
let callHandleReconnect;
beforeEach(() => {
callHandleReconnect = ({
params = makeParams(),
getPrivate = makeGetPrivate(),
StoreAction = makeStoreAction(),
}) => {
// noinspection JSCheckFunctionSignatures
const handleReconnect = makeHandleReconnect(getPrivate, StoreAction);
return handleReconnect(params);
};
});
test("responds if secret key is found", () => {
const params = makeParams();
callHandleReconnect({ params });
expect(params.respond.mock.calls.length).toBe(1);
});
test("updates player's peer id if secret key is found", () => {
const params = makeParams();
const StoreAction = makeStoreAction();
callHandleReconnect({ params, StoreAction });
const { mock } = StoreAction.setPlayerConnection;
expect(mock.calls.length).toBe(1);
const [playerId, { peerId }] = mock.calls[0];
expect(playerId).toBe("player-id");
expect(peerId).toBe("123");
});
test("errors if secret key is not found", () => {
const params = makeParams({
payload: { secretKey: "missing-secret" },
});
callHandleReconnect({ params });
expect(params.error.mock.calls.length).toBe(1);
});
I'm open to any and all feedback on the project as a whole, but specifically I am interested in:
- If my strategy makes sense or if it's way off base
- How it can be improved
- How my tests can be made more readable/improved