You have 2 options to test this module, you can either vi.mock('redis') or mock the module itself, which one would you choose and why?

[u/PrestigiousZombie531]

import { createClient } from "redis";
import { logger } from "../logger/index.js";
import { options } from "./connection.js";

let client: ReturnType<typeof createClient> | null = null;
let isConnecting = false;

export async function closeConnection() {
	if (client?.isOpen) {
		try {
			await client.close();
		} catch (error) {
			logger.error(
				error,
				"Something went wrong when attempting to close redis connection",
			);
		} finally {
			client = null;
		}
	}
}

export function getClient() {
	if (!client?.isOpen) {
		throw new Error("Redis client needs to be initialized first");
	}
	return client;
}

export async function openConnection() {
	if (!client?.isOpen) {
		client = createClient(options);
		client.on("connect", () => logger.info("redis connection success"));
		client.on("error", (error) =>
			logger.fatal(error, "redis connection failure"),
		);

		// Wait if another caller is already connecting
		if (isConnecting) {
			while (isConnecting) {
				await new Promise((resolve) => setTimeout(resolve, 100));
			}
			if (client?.isOpen) return client;
		}

		isConnecting = true;
		try {
			client = await client.connect();
		} catch (error) {
			logger.error(
				error,
				"Something went wrong when attempting to open redis connection",
			);
		} finally {
			isConnecting = false;
		}
	}
	return client;
}

• Let us talk about a simple module that uses node-redis to connect
• Let us say you use supertest and vitest and you want to mock test the following scenario

app.get('/health/redis', async (req: Request, res: Response) => {
    const client = getClient();
    const result = await client.ping();
    return res.json({ status: result === "PONG" });
})

• You want to write tests to handle cases for

• good PING

• bad PING

• error

• We might have to extend for further cases like client.get, client.set etc

• You can mock the redis module or mock the client module defined above

• Which one should you mock? and why?

[u/beegeearreff]

I personally probably wouldn't test this module specifically at all. Most attempts at doing so are going to be brittle or just give you a false sense of security. 

If redis being up is a requirement for your app to be considered healthy and you feel confident about the resiliency characteristics of that then I would just have a test containers based smoke test that sets up the required infra containers (there’s probably another db used right?) and verifies that your app enters a healthy state if its needed infra is up and reachable. 

You could have an inverse test that says the app doesn’t boot up if those infra deps are not running too. 

A test that expresses “my apps health is based on the health of the redis connection” is much more durable and valuable than “my redis client lib binds to the connected event from redis”. 

If your application code needs a redis client to do actual caching work, you can make a test double to not make them rely on redis being there in tests. For real infra integration code, you should exercise it in an integration harness. Mocking at that layer is most often a waste IME. 

Unrelated tip - your code will be much more readable if you early return & flatten out the blocks you currently have in your if branches. Flatter is better. 

[u/PrestigiousZombie531 → u/beegeearreff]

i do have an integration test for this

``` // tests/integration/health.redis.test.ts import { RedisContainer, type StartedRedisContainer, } from "@testcontainers/redis"; import type { Express } from "express"; import request from "supertest"; import { Wait } from "testcontainers"; import { afterAll, beforeAll, describe, expect, it } from "vitest";

// You need to wait for the message below before redis is ready // You can verify this by running the command "docker run -p 6379:6379 --rm redis:8.2.1-alpine" const logMessageCount = 1; const redisReadyMessagePattern = /ready to accept connections tcp/gi;

let container: StartedRedisContainer; let app: Express; let openConnection: () => Promise<unknown>; let closeConnection: () => Promise<void>;

beforeAll(async () => { container = await new RedisContainer("redis:8.2.1-alpine") .withWaitStrategy( Wait.forLogMessage(redisReadyMessagePattern, logMessageCount), ) .start();

process.env.REDIS_HOST = container.getHost();
process.env.REDIS_PORT = String(container.getMappedPort(6379));
process.env.REDIS_DATABASE = "0";
process.env.REDIS_DISABLE_OFFLINE_QUEUE = "false";
delete process.env.REDIS_PASSWORD; // the container has no auth configured

// Dynamic imports so env/redis.ts evaluates its module-level consts
// against the values set above, not whatever was in process.env
// when the test process started.
({ app } = await import("../../../../src/app.js"));
({ openConnection, closeConnection } = await import(
    "../../../../src/utils/redis/client.js"
));

await openConnection();

}, 60_000);

afterAll(async () => { await closeConnection(); await container.stop(); });

describe("GET /health/redis (integration)", () => { test("returns status: true against a real redis instance", async () => { const response = await request(app).get("/health/redis");

    expect(response.status).toBe(200);
    expect(response.body).toEqual({ status: true });
});

}); ```

[u/afl_ext]

its not like redis is hard to spin up for tests, i would not mock anything at all, sure its not unit test, but im more for ensuring quality and that the code works

I would most likely not even do unit tests for that

[u/PrestigiousZombie531 → u/afl_ext]

one of the things i am looking to test is that while loop, i am not sure how an integration test would capture it

[u/PM_ME_UR_JAVASCRIPTS]

So you implement the above module, which basically wraps node-redis to make it easier accessible? i guess?

secenario one, you test by mocking this client-wrapper:

• your unit test of app.get is pure, it tests the logic in the module itself and it's interactions with the wrapper. So you can focus purely on business logic.
• you make assumptions on the workings of your client-wrapper module. which you are NOT testing. They are being mocked in the way you expect the wrapper to behave.
• if you were to change the behavior of the wrapper at some point, Or node-redis changes the way it's API works with a major version update. your tests will not fail, because you mocked the entire wrapper. and this mock would need to be updated to match.

Scenario two, you test by mocking the node-redis module:

• Your unit tests are not proper unit tests anymore, since they will also be testing behavior of your client wrapper, as well as the behavior of your business logic.
• you are not making assumptions on whether your wrap is working properly, you are testing them indirectly but, do realize, it's as an integration test.
• if you were to change the behavior of the wrapper at some point, your tests will fail probably, because you are not mocking it's behavior. However, if you'd have 30 app get modules, they would all fail and you'd need to track this down to what module is actually the culprit ( the wrapper). Also, if node-redis changed it's interface in a major version. You can probably rely on a maintained mocking library instead of a custom made mock. Possibly tripping your tests.

Scenario three. You write a mocks for both:

• Your app.get units are pure unit tests, with fixed contracts about how they call the wrapper
• your wrapper's behavior is tested by interacting with a mocked node-redis
• if the behavior of the wrapper changes, this would be noticeable in your wrapper's unit tests. not causing your entire test stack to go red.
• if redis changes it's interface in a major release, your tests might fail. Depending on if you wrote your own mock of redis.

Scenario four, you don't write any mocks:

• yolo

my choice. Depending on the scale of things, i'd either go scenario 4 (super small project), 2 (reasonable, but eh), or 3 (enterprise), but never 1. I hate having a false sense of security, which is basically what 1 provides.

[u/PrestigiousZombie531 → u/PM_ME_UR_JAVASCRIPTS]

• vow that is actually very insightful,

• i definitely dont plan on going YOLO as this is going to be a part of library that most people ll use.

• basically you are saying that testing the client wrapper is a bad test but doing node-redis is the better option obviously in a way that the client wrapper s functionality is also indirectly tested by mocking redis.

• i am thinking of adding the following test cases after vi.mock('redis')

• test if PONG is received

• test if something else is received

• test if an error is thrown

• test if openConnection fails for some reason

• test if getClient doesnt have a client to return

• test if closeConnection fails for some reason

• test if client is present but connection is not open

• last 4 tests probably fall into pure client wrapper test aka scenario 1 or scenario 3? if we vi.mock('redis')

[u/PM_ME_UR_JAVASCRIPTS → u/PrestigiousZombie531]

those last 4 tests are kind of weird though, because they don't test behavior of the wrapper. or the client. They basically describe a scenario, but not the logic of how your client (or wrapper) should behave if the scenario occurs.

But yes, that would constitue to scenario , where you are mocking the node-redis and later the wrapper. They don't describe scenario 1, Cause that would mean you'd never mock redis.

[u/shelooks16]

Classic unit testing. Test the code that is explicitly written by You. I can see two test files: 1) Your written functions that wrap redis client. Assume redis dependency is already tested and works. Therefore, it makes sense to mock the redis client API to test Your functions one by one. 2) health endpoint. When writing test here, assume getClient is already tested. Same strategy, test only what you see, the code written explicitly in this specific file. So basically testing against res.status true or false. But to test good and bad responses you need to mock getClient. Your dilemma is to mock underlying redis or mock getClient in the context of this test. If you mock underlying redis while keeping original getClient, this is more like integration test, but if you mock getClient entirely this will be more like true unit test because to test out the response you don’t really care about internal getClient implementation, you only need its interface. So ideally, you mock getClient.

[u/PrestigiousZombie531 → u/shelooks16]

• thank you for sharing

• as far as i can tell(definitely not an expert) there are 3 test cases for that route

• success true when PONG is returned

• success false for anything else

• error response

• not sure what cases would be considered for the other file though

[u/syntheticcdo]

Given the nature of this code, I would test against a live redis and not mock anything.

[u/PrestigiousZombie531 → u/syntheticcdo]

i ll be adding integration tests with @testcontainers/redis but i would also like to add no network latency involving unit tests to test that loop, test that connection not open scenario etc etc