Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
246 changes: 246 additions & 0 deletions src/service/metrics/requestHandlerLabel.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,246 @@
import { requestHandlerLabel, UNNAMED_REQUEST_HANDLER } from './requestHandlerLabel'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functional.Support] 🔴 BLOCK

The test file imports requestHandlerLabel and UNNAMED_REQUEST_HANDLER from './requestHandlerLabel', but no such module exists in src/service/metrics/ (the directory contains only client.ts, clusterMetricsAggregator.ts, metrics.ts, otelRequestMetricsMiddleware.ts and requestMetricsMiddleware.ts), and this PR adds no implementation file. The suite will fail to compile and the build/CI will break.

Action: Include src/service/metrics/requestHandlerLabel.ts (exporting requestHandlerLabel and UNNAMED_REQUEST_HANDLER) in this PR, or point the import at the module that actually holds this helper before merging.
Source: https://github.com/vtex/node-vtex-api/tree/master/src/service/metrics

To dismiss: /dk-review dismiss 3f8c1a92-6d47-4b0e-9c15-72ae4d0b8f31 [reason]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[general.broken-references] 🔵 SUGGEST

The relative import './requestHandlerLabel' could not be resolved against the repository's current contents. Please double-check that the referenced module is part of this change set or already merged on the target branch.

Action: Verify the import path resolves on the target branch, and run the test suite locally (yarn test src/service/metrics) to confirm the reference is valid.

To dismiss: /dk-review dismiss c74b52d0-9e11-4a63-8f2c-15d9a6371b48 [reason]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Evolvability.Organizational] 🔵 SUGGEST

The new test is placed as a sibling of the source module, while the existing convention in this package puts specs under src/service/metrics/tests/ (see clusterMetricsAggregator.test.ts). Mixing both layouts makes tests harder to locate and risks diverging jest testMatch/roots coverage.

Action: Move the file to src/service/metrics/tests/requestHandlerLabel.test.ts to match the existing layout, or confirm the jest config intentionally supports co-located specs.
Source: https://github.com/vtex/node-vtex-api/tree/master/src/service/metrics/__tests__

To dismiss: /dk-review dismiss 9a1e6c47-2b83-4d59-a0f7-6e3c81d94520 [reason]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functional.Interface] 🔴 BLOCK

The test file imports requestHandlerLabel and UNNAMED_REQUEST_HANDLER from './requestHandlerLabel', but that module does not exist in the repository and is not added by this PR (the diff contains exactly one file, +246/-0). src/service/metrics/ currently contains only client.ts, clusterMetricsAggregator.ts, metrics.ts, otelRequestMetricsMiddleware.ts and requestMetricsMiddleware.ts. ts-jest will fail to resolve the module, so yarn ci:test (and therefore the whole coverage run) breaks — a hard CI regression rather than a coverage improvement.

Action: Include src/service/metrics/requestHandlerLabel.ts (exporting requestHandlerLabel and UNNAMED_REQUEST_HANDLER) in this PR, or point the import at the module that actually implements the label fallback (e.g. the helper inside requestMetricsMiddleware.ts / otelRequestMetricsMiddleware.ts).
Source: https://github.com/vtex/node-vtex-api/tree/master/src/service/metrics

To dismiss: /dk-review dismiss 3f8c1a24-9d6b-4e21-b7a5-0c1e9d4f7a63 [reason]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functional.Support] 🔵 SUGGEST

The test is placed at src/service/metrics/requestHandlerLabel.test.ts, while the established convention in this package is a __tests__ sibling directory (src/service/metrics/__tests__/clusterMetricsAggregator.test.ts). Jest's testMatch accepts both patterns so the file still runs, but the split location makes the suite harder to discover and diverges from the rest of the module. Golden Path returned no repository rule covering test layout, so this is a convention-only concern.

Action: Move the file to src/service/metrics/__tests__/requestHandlerLabel.test.ts to match the existing layout for this module.
Source: https://github.com/vtex/node-vtex-api/tree/master/src/service/metrics/__tests__

To dismiss: /dk-review dismiss b41d7e05-2c8a-4f19-9a3d-6e5b8c027f14 [reason]

describe('requestHandlerLabel', () => {
describe('UNNAMED_REQUEST_HANDLER constant', () => {
it('should be defined as the string "undefined"', () => {
// Arrange & Act & Assert
expect(UNNAMED_REQUEST_HANDLER).toBe('undefined')
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Evolvability.SolutionApproach] 🔵 SUGGEST

Tests hardcode the literal 'undefined' for the fallback (line 7 and again at the end of the 'requestHandlerName is undefined' case), duplicating the exported constant they are meant to abstract. Changing UNNAMED_REQUEST_HANDLER to a safer sentinel (e.g. 'unnamed') would break tests for reasons unrelated to behaviour, and the assertion pair toBe(UNNAMED_REQUEST_HANDLER) + toBe('undefined') is redundant.

Action: Keep a single test that documents the constant's literal value, and assert only against UNNAMED_REQUEST_HANDLER everywhere else. Also consider whether the sentinel string 'undefined' is the right metric-label value, since it is indistinguishable from an accidentally stringified undefined.

To dismiss: /dk-review dismiss b58347ca-1f92-4e07-8d6b-90a2c4e51f7d [reason]


it('should be a non-empty string', () => {
// Arrange & Act & Assert
expect(typeof UNNAMED_REQUEST_HANDLER).toBe('string')
expect(UNNAMED_REQUEST_HANDLER.length).toBeGreaterThan(0)
})
})

describe('requestHandlerLabel function', () => {
describe('happy path - defined handler names', () => {
it('should return the provided handler name when a non-empty string is given', () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Evolvability.Organizational] 🔵 SUGGEST

Large blocks of the suite are duplicated assertions that add no branch coverage over a pure one-line fallback: five 'happy path' cases plus five 'edge case' cases all exercise the same identity return, the 'consistency and determinism' block re-invokes a stateless pure function three times, and the 'type safety' block asserts typeof result === 'string', which the function's TypeScript return type already guarantees at compile time. This inflates the coverage number without increasing real defect detection and creates maintenance weight for a trivial helper.

Action: Collapse the repeated pass-through cases into a single it.each table, and drop the 'consistency and determinism' and 'type safety' blocks (and expect(UNNAMED_REQUEST_HANDLER.length).toBeGreaterThan(0)), keeping the two behaviourally meaningful cases: a defined name is returned verbatim, and empty-string/undefined fall back to UNNAMED_REQUEST_HANDLER.

To dismiss: /dk-review dismiss 9a2e6b73-5f14-4c8d-8b06-31d7a9e4c250 [reason]

// Arrange

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Evolvability.Organizational] 🔵 SUGGEST

The 'happy path' and 'edge cases' blocks contain ~10 near-identical tests ('getUser', 'listItems', 'api:v1/getUserById', 'handle_user_v2_request', 'list-active-users', 'a', '000handler', '/api/v1.0/endpoint') that all exercise the same single pass-through branch, plus a 'consistency and determinism' block asserting a pure function is deterministic and a 'type safety' block re-asserting typeof === 'string' for a value TypeScript already types as string. This is duplicated test code with no added branch coverage, and it inflates the maintenance cost of a trivial helper.

Action: Collapse the pass-through cases into a single it.each([...]) table and drop the determinism/typeof blocks, keeping the two behaviour-bearing cases (non-empty name returned verbatim, falsy name replaced by the sentinel) plus the whitespace edge case that documents the intentional non-trimming.

To dismiss: /dk-review dismiss 16fa9b83-5c27-40d1-9e4a-c3b8027e6f95 [reason]

const handlerName = 'getUser'

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe('getUser')
})

it('should return handler name for a simple route', () => {
// Arrange
const handlerName = 'listItems'

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe('listItems')
})

it('should return handler name with special characters', () => {
// Arrange
const handlerName = 'api:v1/getUserById'

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe('api:v1/getUserById')
})

it('should return handler name with numbers and underscores', () => {
// Arrange
const handlerName = 'handle_user_v2_request'

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe('handle_user_v2_request')
})

it('should return handler name with hyphens', () => {
// Arrange
const handlerName = 'list-active-users'

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe('list-active-users')
})
})

describe('fallback to UNNAMED_REQUEST_HANDLER', () => {
it('should return UNNAMED_REQUEST_HANDLER when requestHandlerName is undefined', () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functional.Check] 🔵 SUGGEST

The fallback suite covers undefined, a missing argument and '', but not null. The producer of this value is ctx.requestHandlerName on a Koa context, which is populated at runtime by untyped router code, so null is a realistic input; if the implementation uses ?? instead of || (or vice versa) the null path would silently emit a null metric label and no test would catch it.

Action: Add a case asserting the behaviour for null (cast as needed, e.g. requestHandlerLabel(null as unknown as string)), so the nullish-vs-falsy semantics of the implementation are pinned by a test.

To dismiss: /dk-review dismiss e2d70f16-84a5-4c3b-b19e-5f60c7a8d213 [reason]

// Arrange
const handlerName = undefined

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe(UNNAMED_REQUEST_HANDLER)
expect(result).toBe('undefined')
})

it('should return UNNAMED_REQUEST_HANDLER when no argument is provided', () => {
// Arrange & Act
const result = requestHandlerLabel()

// Assert
expect(result).toBe(UNNAMED_REQUEST_HANDLER)
})

it('should return UNNAMED_REQUEST_HANDLER when an empty string is provided', () => {
// Arrange
const handlerName = ''

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe(UNNAMED_REQUEST_HANDLER)
})
})

describe('edge cases and boundary values', () => {
it('should return a string with only whitespace as the handler name', () => {
// Arrange
const handlerName = ' '

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe(' ')
expect(result).not.toBe(UNNAMED_REQUEST_HANDLER)
})

it('should return a string with a single space character as the handler name', () => {
// Arrange
const handlerName = ' '

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe(' ')
})

it('should return a single character handler name', () => {
// Arrange
const handlerName = 'a'

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe('a')
})

it('should return a very long handler name', () => {
// Arrange
const handlerName = 'a'.repeat(1000)

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe(handlerName)
expect(result.length).toBe(1000)
})

it('should return handler name with leading zeros', () => {
// Arrange
const handlerName = '000handler'

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe('000handler')
})

it('should return handler name with dots and slashes', () => {
// Arrange
const handlerName = '/api/v1.0/endpoint'

// Act
const result = requestHandlerLabel(handlerName)

// Assert
expect(result).toBe('/api/v1.0/endpoint')
})
})

describe('consistency and determinism', () => {
it('should return the same value on repeated calls with the same input', () => {
// Arrange
const handlerName = 'consistentHandler'

// Act
const result1 = requestHandlerLabel(handlerName)
const result2 = requestHandlerLabel(handlerName)
const result3 = requestHandlerLabel(handlerName)

// Assert
expect(result1).toBe(result2)
expect(result2).toBe(result3)
})

it('should return UNNAMED_REQUEST_HANDLER consistently for undefined inputs', () => {
// Arrange & Act
const result1 = requestHandlerLabel(undefined)
const result2 = requestHandlerLabel()
const result3 = requestHandlerLabel(undefined)

// Assert
expect(result1).toBe(result2)
expect(result2).toBe(result3)
expect(result1).toBe(UNNAMED_REQUEST_HANDLER)
})

it('should return UNNAMED_REQUEST_HANDLER consistently for empty strings', () => {
// Arrange & Act
const result1 = requestHandlerLabel('')
const result2 = requestHandlerLabel('')

// Assert
expect(result1).toBe(result2)
expect(result1).toBe(UNNAMED_REQUEST_HANDLER)
})
})

describe('type safety', () => {
it('should return a string type', () => {
// Arrange & Act
const result = requestHandlerLabel('test')

// Assert
expect(typeof result).toBe('string')
})

it('should return a string type even when undefined is passed', () => {
// Arrange & Act
const result = requestHandlerLabel(undefined)

// Assert
expect(typeof result).toBe('string')
})

it('should always return a non-empty string', () => {
// Arrange & Act
const result1 = requestHandlerLabel('handler')
const result2 = requestHandlerLabel('')
const result3 = requestHandlerLabel(undefined)

// Assert
expect(result1.length).toBeGreaterThan(0)
expect(result2.length).toBeGreaterThan(0)
expect(result3.length).toBeGreaterThan(0)
})
})
})
})