From b8822bcbddfb6af48d9a5fd9231be4c329e3b081 Mon Sep 17 00:00:00 2001 From: Vasily Martynov Date: Wed, 16 Oct 2024 17:55:22 +1300 Subject: [PATCH] Refactor HTTP server and add mote tests - Add health route - Handle server close gracefully - Add new methods to storage getMaxStorageSize and getDefaultTTL - Add tests got HTTP server - Add more API tests - Improve test tools - Run tests in one thread - Add placeholder for performance tests - Update express --- .github/workflows/nodejs.yaml | 2 +- jest.config.ts | 7 +- package-lock.json | 16 ++-- package.json | 6 +- src/kvHttpServer.ts | 77 +++++++++++++++---- src/kvStore.ts | 8 ++ src/types.ts | 9 ++- test/performance.test.ts | 65 ++++++++++++++++ test/restApi.test.ts | 48 +++++++++--- test/server.test.ts | 141 ++++++++++++++++++++++++++++++++++ test/storage.test.ts | 4 +- test/testUtils.ts | 26 +++++-- tsconfig.json | 1 - 13 files changed, 360 insertions(+), 50 deletions(-) create mode 100644 test/performance.test.ts create mode 100644 test/server.test.ts diff --git a/.github/workflows/nodejs.yaml b/.github/workflows/nodejs.yaml index 72e8a5d..b7f58d4 100644 --- a/.github/workflows/nodejs.yaml +++ b/.github/workflows/nodejs.yaml @@ -20,4 +20,4 @@ jobs: with: node-version: ${{ matrix.node }} - run: npm ci - - run: npm run test + - run: npm run test:coverage diff --git a/jest.config.ts b/jest.config.ts index 2fe1fa2..743fdb2 100644 --- a/jest.config.ts +++ b/jest.config.ts @@ -2,10 +2,11 @@ import type { Config } from 'jest'; const config: Config = { verbose: true, - testEnvironment: "node", + testEnvironment: 'node', transform: { - "^.+.tsx?$": ["ts-jest", {}], + '^.+.tsx?$': ['ts-jest', {}], }, + maxWorkers: 1, }; -export default config; \ No newline at end of file +export default config; diff --git a/package-lock.json b/package-lock.json index cf08f2e..c93b832 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,7 +9,7 @@ "version": "1.0.0", "license": "MIT", "dependencies": { - "express": "^4.21.0", + "express": "^4.21.1", "typescript": "^5.6.2" }, "devDependencies": { @@ -2021,9 +2021,9 @@ "license": "MIT" }, "node_modules/cookie": { - "version": "0.6.0", - "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.6.0.tgz", - "integrity": "sha512-U71cyTamuh1CRNCfpGY6to28lxvNwPG4Guz/EVjgf3Jmzv0vlDp1atT9eS5dDjMYHucpHbWns6Lwf3BKz6svdw==", + "version": "0.7.1", + "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.7.1.tgz", + "integrity": "sha512-6DnInpx7SJ2AK3+CTUE/ZM0vWTUboZCegxhC2xiIydHR9jNuTAASBrfEpHhiGOZw/nX51bHt6YQl8jsGo4y/0w==", "license": "MIT", "engines": { "node": ">= 0.6" @@ -2396,9 +2396,9 @@ } }, "node_modules/express": { - "version": "4.21.0", - "resolved": "https://registry.npmjs.org/express/-/express-4.21.0.tgz", - "integrity": "sha512-VqcNGcj/Id5ZT1LZ/cfihi3ttTn+NJmkli2eZADigjq29qTlWi/hAQ43t/VLPq8+UX06FCEx3ByOYet6ZFblng==", + "version": "4.21.1", + "resolved": "https://registry.npmjs.org/express/-/express-4.21.1.tgz", + "integrity": "sha512-YSFlK1Ee0/GC8QaO91tHcDxJiE/X4FbpAyQWkxAvG6AXCuR65YzK8ua6D9hvi/TzUfZMpc+BwuM1IPw8fmQBiQ==", "license": "MIT", "dependencies": { "accepts": "~1.3.8", @@ -2406,7 +2406,7 @@ "body-parser": "1.20.3", "content-disposition": "0.5.4", "content-type": "~1.0.4", - "cookie": "0.6.0", + "cookie": "0.7.1", "cookie-signature": "1.0.6", "debug": "2.6.9", "depd": "2.0.0", diff --git a/package.json b/package.json index 2a13dd0..b5ff98d 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,9 @@ "build": "tsc -p .", "start": "node ./dist/index.js", "dev": "npm run build && npm run start", - "test": "jest --coverage" + "test": "jest --detectOpenHandles", + "test:coverage": "jest --coverage --detectOpenHandles" + }, "repository": { "type": "git", @@ -23,7 +25,7 @@ "author": "Vasily Martynov", "license": "MIT", "dependencies": { - "express": "^4.21.0", + "express": "^4.21.1", "typescript": "^5.6.2" }, "devDependencies": { diff --git a/src/kvHttpServer.ts b/src/kvHttpServer.ts index 64b9c1c..77f09ad 100644 --- a/src/kvHttpServer.ts +++ b/src/kvHttpServer.ts @@ -1,7 +1,7 @@ -import express from 'express'; +import express, { Request, Response, NextFunction } from 'express'; import http from 'http'; import KVStore from './kvStore'; -import { GetResponse, InvalidRequestError, PutResponse } from './types'; +import { GetResponse, InvalidRequestError, PutResponse, Stats } from './types'; import { AddressInfo } from 'net'; class KVServer { @@ -17,33 +17,67 @@ class KVServer { this.store = new KVStore(maxStorageSize, defaultTTL); } + private handlePutResponse(res: Response, putRes: PutResponse) { + if (putRes.putResult) { + console.log(`return 1`); + res.status(201).json(putRes); + return; + } + console.log(`return 2`); + res.status(432).json(putRes); + } + private registerRoutes() { this.app.use(express.json()); - this.app.use((_req, res, next) => { + this.app.use((_req: Request, res: Response, next: NextFunction) => { res.header('Content-Type', 'application/json'); next(); }); - this.app.route('/kv/v1/get/:key').get((req, res) => { + this.app.route('/kv/v1/health').get((_req: Request, res: Response) => { + const healthResp: Stats = { + defaultTTL: this.store.getDefaultTTL(), + maxStorageSize: this.store.getMaxStorageSize(), + storageUsed: this.store.getSize(), + }; + res.json(healthResp); + }); + + this.app.route('/kv/v1/get/:key').get((req: Request, res: Response) => { + const key = req.params['key']; + + if (!key) { + const errResp: InvalidRequestError = { + error: `Invalid request`, + message: `key must be string or number`, + }; + res.status(400).json(errResp); + return; + } + const getResp: GetResponse = { - data: this.store.get(req.params.key) || null, + data: this.store.get(key) || null, }; + if (getResp.data) { + res.status(200).json(getResp); + return; + } + res.status(200).json(getResp); res.json(getResp); }); - this.app.route('/kv/v1/put/:key').post((req, res) => { + this.app.route('/kv/v1/put/:key').post((req, res: Response) => { const putResp: PutResponse = { - result: this.store.set(req.params.key, req.body), + putResult: this.store.set(req.params.key, req.body), }; - res.json(putResp); + this.handlePutResponse(res, putResp); }); - this.app.route('/kv/v1/put/:key/:ttl').post((req, res) => { + this.app.route('/kv/v1/put/:key/:ttl').post((req, res: Response) => { const ttl = Number(req.params.ttl); if (isNaN(ttl)) { const errResp: InvalidRequestError = { - code: 400, error: `Invalid request`, message: `ttl parameter must be a number`, }; @@ -51,9 +85,10 @@ class KVServer { } const putResp: PutResponse = { - result: this.store.set(req.params.key, req.body, ttl), + putResult: this.store.set(req.params.key, req.body, ttl), }; - res.json(putResp); + + this.handlePutResponse(res, putResp); }); } @@ -67,8 +102,22 @@ class KVServer { ); } - public stop() { - this.server?.close(); + public async stop() { + if (this.server) { + let isClosed = false; + + this.server.close((err: unknown) => { + if (err) { + console.log(`Failed to stop server`, err); + } + isClosed = true; + }); + + const waitUntil = Date.now() + 5000; + while (waitUntil > Date.now() && !isClosed) { + await new Promise((resolve) => setTimeout(resolve, 100)); + } + } } public getInstance() { diff --git a/src/kvStore.ts b/src/kvStore.ts index c5665be..954fd2a 100644 --- a/src/kvStore.ts +++ b/src/kvStore.ts @@ -76,6 +76,14 @@ class KVStore { public getSize(): number { return this.store.size; } + + public getMaxStorageSize(): number { + return this.maxStorageSize; + } + + public getDefaultTTL(): number { + return this.defaultTTL; + } } export default KVStore; diff --git a/src/types.ts b/src/types.ts index d35592a..326d5c6 100644 --- a/src/types.ts +++ b/src/types.ts @@ -3,11 +3,16 @@ export type GetResponse = { }; export type PutResponse = { - result: boolean; + putResult: boolean; }; export type InvalidRequestError = { - code: 400; error: string; message: string; }; + +export type Stats = { + defaultTTL: number; + maxStorageSize: number; + storageUsed: number; +}; diff --git a/test/performance.test.ts b/test/performance.test.ts new file mode 100644 index 0000000..16d2333 --- /dev/null +++ b/test/performance.test.ts @@ -0,0 +1,65 @@ +import KVStore from '../src/kvStore'; + +function getPercentile(latencyArr: number[], percentile: number) { + latencyArr.sort((a, b) => a - b); + const index = Math.ceil((percentile / 100) * latencyArr.length) - 1; + return latencyArr[index]; +} + +describe('Storage performance', () => { + test.skip('Add 1M items', async () => { + const store = new KVStore(1000000); + + console.log(`>>> memory usage`, process.memoryUsage()); + + for (let i = 0; i < 1000000; i++) { + store.set(i, { someData: 'test data' }); + } + + console.log(`>>> memory usage`, process.memoryUsage()); + + const latency: number[] = []; + + for (let i = 0; i < 1000000; i++) { + const start = performance.now(); + store.set(i, { someData: 'test data' }); + const end = performance.now(); + + latency.push(Math.ceil(end - start)); + } + + console.log(`>>> memory usage`, process.memoryUsage()); + + const p90 = getPercentile(latency, 90); + console.log(`90th percentile: ${p90} ms`); + const p95 = getPercentile(latency, 95); + console.log(`95th percentile: ${p95} ms`); + const p99 = getPercentile(latency, 99); + console.log(`99th percentile: ${p99} ms`); + }); + + test.skip('Get 1M items', async () => { + const store = new KVStore(1000000); + + for (let i = 0; i < 1000000; i++) { + store.set(i, { someData: 'test data' }); + } + + const latency: number[] = []; + + for (let i = 0; i < 1000000; i++) { + const start = performance.now(); + store.get(i); + const end = performance.now(); + + latency.push(Math.ceil(end - start)); + } + + const p90 = getPercentile(latency, 90); + console.log(`90th percentile: ${p90} ms`); + const p95 = getPercentile(latency, 95); + console.log(`95th percentile: ${p95} ms`); + const p99 = getPercentile(latency, 99); + console.log(`99th percentile: ${p99} ms`); + }); +}); diff --git a/test/restApi.test.ts b/test/restApi.test.ts index 90b892b..9553ec8 100644 --- a/test/restApi.test.ts +++ b/test/restApi.test.ts @@ -32,21 +32,38 @@ describe('Storage test', () => { }); }); - test('Put new item and then get existing item Axios', async () => { + test('Get item that does not exist, key is number', async () => { + await request(server.getInstance()) + .get('/kv/v1/get/123456') + .expect('Content-Type', 'application/json; charset=utf-8') + .expect(200) + .then((res: Response) => { + const apiResp: PutResponse = res.body; + expect(apiResp).toEqual({ data: null }); + }); + }); + + test('Put new item then get existing item, key is a string (Axios)', async () => { await putItem(urlPut, 'key1', { item1: 1, item2: 'two' }, 1000); const resp = await getItem(urlGet, 'key1'); expect(resp).toEqual({ data: { item1: 1, item2: 'two' } }); }); - test('Put new item and then get existing item', async () => { + test('Put new item then get existing item, key is a number (Axios)', async () => { + await putItem(urlPut, '123', { item1: 1, item2: 'two' }, 1000); + const resp = await getItem(urlGet, '123'); + expect(resp).toEqual({ data: { item1: 1, item2: 'two' } }); + }); + + test('Put new item then get existing item', async () => { await request(server.getInstance()) .post('/kv/v1/put/key2') .send({ item1: 1, item2: 'two' }) .expect('Content-Type', 'application/json; charset=utf-8') - .expect(200) + .expect(201) .then((res: Response) => { const apiResp: PutResponse = res.body; - expect(apiResp).toEqual({ result: true }); + expect(apiResp).toEqual({ putResult: true }); }); await request(server.getInstance()) .get('/kv/v1/get/key2') @@ -63,10 +80,10 @@ describe('Storage test', () => { .post('/kv/v1/put/key60000/60000') .send({ item1: 1, item2: 'two' }) .expect('Content-Type', 'application/json; charset=utf-8') - .expect(200) + .expect(201) .then((res: Response) => { const apiResp: PutResponse = res.body; - expect(apiResp).toEqual({ result: true }); + expect(apiResp).toEqual({ putResult: true }); }); }); @@ -79,10 +96,24 @@ describe('Storage test', () => { .post('/kv/v1/put/keyThatShouldExpire/60000') .send({ item1: 1, item2: 'two' }) .expect('Content-Type', 'application/json; charset=utf-8') - .expect(200) + .expect(201) + .then((res: Response) => { + const apiResp: PutResponse = res.body; + expect(apiResp).toEqual({ putResult: true }); + }); + }); + + test('Put item does not override existing item if TTL not expired', async () => { + await putItem(urlPut, 'keyNotExpired', { test: 'test' }, 10000); + + await request(server.getInstance()) + .post('/kv/v1/put/keyNotExpired/60000') + .send({ item1: 1, item2: 'two' }) + .expect('Content-Type', 'application/json; charset=utf-8') + .expect(432) .then((res: Response) => { const apiResp: PutResponse = res.body; - expect(apiResp).toEqual({ result: true }); + expect(apiResp).toEqual({ putResult: false }); }); }); @@ -95,7 +126,6 @@ describe('Storage test', () => { .then((res: Response) => { const apiResp: InvalidRequestError = res.body; expect(apiResp).toEqual({ - code: 400, error: 'Invalid request', message: 'ttl parameter must be a number', }); diff --git a/test/server.test.ts b/test/server.test.ts new file mode 100644 index 0000000..7f3627c --- /dev/null +++ b/test/server.test.ts @@ -0,0 +1,141 @@ +import KVServer from '../src/kvHttpServer'; +import { PutResponse } from '../src/types'; +import { getHealth, getItem, putItem, retryAssertEqual, sleep } from './testUtils'; +import request, { Response } from 'supertest'; + +describe('Storage test', () => { + let server: KVServer; + + const defaultPort = 8080; + const nonDefaultPort = 8081; + + const urlPutDefault = `http://localhost:${defaultPort}/kv/v1/put`; + const urlGetDefault = `http://localhost:${defaultPort}/kv/v1/get`; + const urlHealthDefault = `http://localhost:${defaultPort}/kv/v1/health`; + + const urlPut = `http://localhost:${nonDefaultPort}/kv/v1/put`; + const urlGet = `http://localhost:${nonDefaultPort}/kv/v1/get`; + const urlHealth = `http://localhost:${nonDefaultPort}/kv/v1/health`; + + afterEach(async () => { + server.stop(); + }); + + test('Can override default max storage size', async () => { + server = new KVServer(2, 9999, nonDefaultPort); + server.start(); + await retryAssertEqual(() => getHealth(urlHealth), { + defaultTTL: 9999, + maxStorageSize: 2, + storageUsed: 0, + }); + await putItem(urlPut, 'key1', { item1: 1 }, 1000); + await putItem(urlPut, 'key2', { item1: 2 }, 1000); + await request(urlPut) + .post('/key3/1000') + .send({ item1: 1, item2: 'two' }) + .expect('Content-Type', 'application/json; charset=utf-8') + .expect(432) + .then((res: Response) => { + const apiResp: PutResponse = res.body; + expect(apiResp).toEqual({ putResult: false }); + }); + + expect(await getHealth(urlHealth)).toEqual({ + defaultTTL: 9999, + maxStorageSize: 2, + storageUsed: 2, + }); + + const resp = await getItem(urlGet, 'key1'); + expect(resp).toEqual({ data: { item1: 1 } }); + const resp2 = await getItem(urlGet, 'key2'); + expect(resp2).toEqual({ data: { item1: 2 } }); + const resp3 = await getItem(urlGet, 'key3'); + expect(resp3).toEqual({ data: null }); + }); + + test('Can override default ttl', async () => { + server = new KVServer(2, 10, nonDefaultPort); + server.start(); + await retryAssertEqual(() => getHealth(urlHealth), { + defaultTTL: 10, + maxStorageSize: 2, + storageUsed: 0, + }); + await putItem(urlPut, 'key1', { item1: 1 }); + await sleep(11); + await request(urlPut) + .post('/key1') + .send({ item1: 1, item2: 'two' }) + .expect('Content-Type', 'application/json; charset=utf-8') + .expect(201) + .then((res: Response) => { + const apiResp: PutResponse = res.body; + expect(apiResp).toEqual({ putResult: true }); + }); + + expect(await getHealth(urlHealth)).toEqual({ + defaultTTL: 10, + maxStorageSize: 2, + storageUsed: 1, + }); + + const resp = await getItem(urlGet, 'key1'); + expect(resp).toEqual({ data: { item1: 1, item2: 'two' } }); + }); + + test('Can use default port', async () => { + server = new KVServer(2, 10); + server.start(); + await retryAssertEqual(() => getHealth(urlHealthDefault), { + defaultTTL: 10, + maxStorageSize: 2, + storageUsed: 0, + }); + await putItem(urlPutDefault, 'key1', { item1: 1 }); + const resp = await getItem(urlGetDefault, 'key1'); + expect(resp).toEqual({ data: { item1: 1 } }); + expect(await getHealth(urlHealthDefault)).toEqual({ + defaultTTL: 10, + maxStorageSize: 2, + storageUsed: 1, + }); + }); + + test('Can use default port and default ttl', async () => { + server = new KVServer(2); + server.start(); + await retryAssertEqual(() => getHealth(urlHealthDefault), { + defaultTTL: 60000, + maxStorageSize: 2, + storageUsed: 0, + }); + await putItem(urlPutDefault, 'key1', { item1: 1 }); + const resp = await getItem(urlGetDefault, 'key1'); + expect(resp).toEqual({ data: { item1: 1 } }); + expect(await getHealth(urlHealthDefault)).toEqual({ + defaultTTL: 60000, + maxStorageSize: 2, + storageUsed: 1, + }); + }); + + test('Can use all default properties', async () => { + server = new KVServer(); + server.start(); + await retryAssertEqual(() => getHealth(urlHealthDefault), { + defaultTTL: 60000, + maxStorageSize: 1000000, + storageUsed: 0, + }); + await putItem(urlPutDefault, 'key1', { item1: 1 }); + const resp = await getItem(urlGetDefault, 'key1'); + expect(resp).toEqual({ data: { item1: 1 } }); + expect(await getHealth(urlHealthDefault)).toEqual({ + defaultTTL: 60000, + maxStorageSize: 1000000, + storageUsed: 1, + }); + }); +}); diff --git a/test/storage.test.ts b/test/storage.test.ts index 48b8844..5f637e5 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -1,5 +1,5 @@ import KVStore from '../src/kvStore'; -import { retry, sleep } from './testUtils'; +import { retryAssertEqual, sleep } from './testUtils'; describe('Storage test', () => { test('Get item that does not exist', async () => { @@ -24,7 +24,7 @@ describe('Storage test', () => { expect(res2).toEqual(false); expect(store.get('key1')).toEqual({ someData: 'test data' }); - await retry(() => expect(store.set('key1', { someData: 'new data' }, 200)).toEqual(true)); + await retryAssertEqual(() => store.set('key1', { someData: 'new data' }, 200), true); expect(store.get('key1')).toEqual({ someData: 'new data' }); }); diff --git a/test/testUtils.ts b/test/testUtils.ts index e11169c..f72dc3e 100644 --- a/test/testUtils.ts +++ b/test/testUtils.ts @@ -9,21 +9,23 @@ export const sleep = async (timeout: number) => { }; /** - * Retry to execute function within specified timeout, exit on success + * Retry to execute function and compare result with provided value within specified timeout, exit on success * @param func function to execute * @param timeout timeout in millisecinds * @returns */ -export const retry = async (func: () => void, timeout = 1000) => { +export const retryAssertEqual = async (func: () => unknown, expectedValue: unknown, timeout = 5000) => { const startedAt = Date.now(); let error: Error = new Error(`Retry did not run`); while (startedAt + timeout > Date.now()) { try { - func(); + const actualValue = await func(); + expect(actualValue).toEqual(expectedValue); console.warn(`Assertion succeeded after ${Date.now() - startedAt} ms`); return; } catch (err: unknown) { - error = err instanceof Error ? err : new Error(`Unknow error`); + error = err instanceof Error ? err : new Error(`Unknown error`); + console.warn(`Assert failed, re-trying`, err); await sleep(100); } } @@ -37,8 +39,8 @@ export const retry = async (func: () => void, timeout = 1000) => { * @param key */ export async function getItem(url: string, key: string): Promise { - const putUrl = `${url}/${key}`; - const response = await axios.get(putUrl); + const getUrl = `${url}/${key}`; + const response = await axios.get(getUrl, { timeout: 100 }); return response.data; } @@ -46,8 +48,6 @@ export async function getItem(url: string, key: string): Promise { * Helper to put item * @param url * @param key - * @param obj - * @param ttl in milliseconds */ export async function putItem(url: string, key: string, obj: unknown, ttl?: number): Promise { const putUrl = `${url}/${key}${ttl ? '/' + ttl : ''}`; @@ -55,6 +55,16 @@ export async function putItem(url: string, key: string, obj: unknown, ttl?: numb headers: { 'Content-Type': 'application/json; charset=utf-8', }, + timeout: 100, }); return response.data; } + +/** + * Helper to get server stats + * @param url + */ +export async function getHealth(url: string): Promise { + const response = await axios.get(url, { timeout: 100 }); + return response.data; +} diff --git a/tsconfig.json b/tsconfig.json index 6c882c3..74d705f 100755 --- a/tsconfig.json +++ b/tsconfig.json @@ -30,7 +30,6 @@ "noPropertyAccessFromIndexSignature": true, "allowUnusedLabels": true, "allowUnreachableCode": true, - //"skipLibCheck": true }, "include": [ "./src/**/*",