2
\$\begingroup\$

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:

  1. getPrivate is a function that returns the current state of my flux storage.

  2. StoreAction.setPlayerConnection is a function that updates my store. The StoreAction 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:

  1. If my strategy makes sense or if it's way off base
  2. How it can be improved
  3. How my tests can be made more readable/improved
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.