Skip to content
Open
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
279 changes: 279 additions & 0 deletions src/service/metrics/requestHandlerLabel.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,279 @@
import { UNNAMED_REQUEST_HANDLER, requestHandlerLabel } 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.

[general.broken-references] 🔵 SUGGEST

The test imports UNNAMED_REQUEST_HANDLER and requestHandlerLabel from './requestHandlerLabel', but src/service/metrics/requestHandlerLabel.ts does not exist on master and is not part of the reviewed diff. If the source module is not included in this PR, the suite will fail to compile under ts-jest.

Action: Confirm that src/service/metrics/requestHandlerLabel.ts is included in this PR (and that requestHandlerLabel declares its parameter as optional, since line 91 calls it with no argument). Run yarn test locally to verify the module resolves.

To dismiss: /dk-review dismiss b7e3f1a2-4c58-4d19-9f2e-8a6c0d3b7e51 [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 file is placed directly in src/service/metrics/ while the existing test in this module lives in src/service/metrics/__tests__/clusterMetricsAggregator.test.ts. Jest's testRegex picks it up either way, so this is not a functional break, but the split location makes module tests harder to locate.

Action: Move the file to src/service/metrics/__tests__/requestHandlerLabel.test.ts and adjust the import to ../requestHandlerLabel, matching the sibling test.

To dismiss: /dk-review dismiss d3b81f47-5a29-4c60-8e14-9f7c3a02b6e5 [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 UNNAMED_REQUEST_HANDLER and requestHandlerLabel from './requestHandlerLabel', but no such module exists in the repository — src/service/metrics/ contains only client.ts, clusterMetricsAggregator.ts, metrics.ts, otelRequestMetricsMiddleware.ts and requestMetricsMiddleware.ts, and the PR adds no implementation file. The suite will fail to compile (TS2307) and CI will break, so the intended coverage gain is never realized.

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

To dismiss: /dk-review dismiss 6f1a2c84-7d3b-4e5a-9c21-8b0d4e7f1a33 [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 test is placed directly in src/service/metrics/ while the established convention in this module is a __tests__ subdirectory (src/service/metrics/__tests__/clusterMetricsAggregator.test.ts). Split locations make test discovery and jest testPathIgnorePatterns/build-exclusion config error-prone, and a stray test file under src/ can be picked up by the TypeScript build output.

Action: Move the file to src/service/metrics/__tests__/requestHandlerLabel.test.ts and adjust the import to '../requestHandlerLabel'.
Source: https://github.com/vtex/node-vtex-api/tree/master/src/service/metrics/__tests__

To dismiss: /dk-review dismiss b28c5d71-9e04-4a6f-8f13-2c7a5d908e42 [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.

[quality.new-logic-enforcement] 🔵 SUGGEST

The quality-ratchet scenario requires that coverage not regress and that new logic ship with tests. This PR is test-only and adds no production code, so no baseline regression is introduced; however, because the module under test is absent the suite cannot execute, meaning the coverage report will not improve and may fail to generate at all. Note that goldenPathGetRules returned no Golden Path rules for this repository/file pattern, so this is reported as a suggestion rather than a block.

Action: Land the implementation together with these tests and confirm the coverage run passes and reports a non-negative delta against the baseline before merging.

To dismiss: /dk-review dismiss 9c3e8b50-61df-4d27-b4aa-0f2e7c14a6d8 [reason]

describe('requestHandlerLabel', () => {
describe('UNNAMED_REQUEST_HANDLER constant', () => {
it('should export the constant with value "undefined"', () => {
// Arrange & Act & Assert
expect(UNNAMED_REQUEST_HANDLER).toBe('undefined')
})

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', () => {
it('should return the provided requestHandlerName when it is a non-empty string', () => {
// Arrange
const handlerName = 'getUserById'

// Act
const result = requestHandlerLabel(handlerName)

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

it('should return the provided requestHandlerName for various valid handler names', () => {
// Arrange
const testCases = [
'listUsers',
'createPost',
'deleteComment',
'updateProfile',
'builtin:notFound',
'builtin:error',
'middleware:auth'
]

// Act & Assert
testCases.forEach(handlerName => {
expect(requestHandlerLabel(handlerName)).toBe(handlerName)
})
})

it('should handle handler names with special characters', () => {
// Arrange
const specialNames = [
'handler-with-dash',
'handler_with_underscore',
'handler.with.dot',
'handler/with/slash',
'handler:with:colon'
]

// Act & Assert
specialNames.forEach(name => {
expect(requestHandlerLabel(name)).toBe(name)
})
})

it('should handle handler names with numbers', () => {
// Arrange
const nameWithNumbers = 'handler123'

// Act
const result = requestHandlerLabel(nameWithNumbers)

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

describe('edge cases - undefined and null', () => {
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.

[Evolvability.Textual] 🔵 SUGGEST

The describe block is named 'edge cases - undefined and null' but contains no test for a null input — only undefined and the omitted-argument case. The block name promises coverage the suite does not provide, which is misleading for future maintainers auditing coverage.

Action: Either add a requestHandlerLabel(null as any) case asserting the fallback (this module is consumed by plain-JS callers via the published lib/, where null is reachable), or rename the block to 'edge cases - undefined'.

To dismiss: /dk-review dismiss c1d94b6e-2f77-4a03-8b5d-6e2f9c14a8d3 [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)
expect(result).toBe('undefined')
})
})

describe('edge cases - empty strings', () => {
it('should return UNNAMED_REQUEST_HANDLER when requestHandlerName is an empty string', () => {
// Arrange
const handlerName = ''

// Act
const result = requestHandlerLabel(handlerName)

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

it('should fall back for empty string to ensure label is never emitted empty', () => {
// Arrange
const emptyHandler = ''

// Act
const result = requestHandlerLabel(emptyHandler)

// Assert
expect(result).not.toBe('')
expect(result.length).toBeGreaterThan(0)
})
})

describe('edge cases - whitespace-only strings', () => {
it('should NOT fall back for whitespace-only strings (they are truthy)', () => {
// Arrange
const whitespaceHandler = ' '

// Act
const result = requestHandlerLabel(whitespaceHandler)

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

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 test locks in emitting whitespace-only strings (' ', ' ') verbatim as the metric label. Since this value becomes a prom-client label, a whitespace-only handler name produces an invisible, near-undistinguishable time series that will look identical to other whitespace variants in dashboards — the same class of problem the empty-string fallback exists to prevent.

Action: Decide whether the fallback should use a trimmed check (requestHandlerName?.trim() ? requestHandlerName : UNNAMED_REQUEST_HANDLER). If the current truthy-only behaviour is deliberate, add a comment in the test explaining why whitespace labels are acceptable rather than only stating the mechanism ('they are truthy').

To dismiss: /dk-review dismiss e5a20c88-9b41-4c6a-a7f3-1d8e4b90c2f6 [reason]


it('should preserve single space string', () => {
// Arrange
const singleSpace = ' '

// Act
const result = requestHandlerLabel(singleSpace)

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 tests 'should NOT fall back for whitespace-only strings (they are truthy)' and 'should preserve single space string' lock in a whitespace-only value as a valid Prometheus label. The rest of the suite argues the helper exists precisely so the label is never blank; a ' ' label is effectively blank for a human reading a dashboard yet creates a distinct time series, so codifying it as expected behaviour cements a validation gap rather than exposing it.

Action: Decide the intended contract explicitly: either trim the input in requestHandlerLabel and fall back to UNNAMED_REQUEST_HANDLER for whitespace-only names (updating these two tests), or add a comment in the test explaining why whitespace-only names are deliberately preserved.

To dismiss: /dk-review dismiss d4470f16-3a58-4b92-a7e6-51c8b3d6027f [reason]

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

describe('consistency with aggregation expectations', () => {
it('should return consistent string for undefined across multiple calls', () => {

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

Several tests duplicate assertions already made elsewhere in the file: 'should preserve historical series identity' (line 178) is the same undefined case as line 77; 'should ensure label is never empty string in any scenario' (line 162) and 'should fall back for empty string...' (line 112) restate line 100; 'should preserve single space string' (line 137) restates line 126; and 'should return a string in all cases' (line 193) is guaranteed by the TypeScript return type. This inflates the suite without adding coverage and raises the cost of every future change to the function.

Action: Consolidate the overlapping cases into a single table-driven test (e.g. it.each([[undefined, 'undefined'], ['', 'undefined'], ['getUserById', 'getUserById'], [' ', ' ']])) and drop assertions that only re-check the declared type.

To dismiss: /dk-review dismiss f92c6a05-3d84-4e17-9c60-2b5f8a71e0d4 [reason]

// Arrange & Act
const result1 = requestHandlerLabel(undefined)
const result2 = requestHandlerLabel()
const result3 = requestHandlerLabel('')

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

it('should ensure label is never empty string in any scenario', () => {
// Arrange
const scenarios = [
undefined,
'',
'validHandler'
]

// Act & Assert
scenarios.forEach(scenario => {
const result = requestHandlerLabel(scenario)
expect(result).not.toBe('')
expect(result.length).toBeGreaterThan(0)
})
})

it('should preserve historical series identity with "undefined" string', () => {
// Arrange
const undefinedHandler = undefined

// Act
const result = requestHandlerLabel(undefinedHandler)

// Assert
expect(result).toBe('undefined')
// Verify it matches the constant to ensure prom-client serialization consistency
expect(result).toBe(UNNAMED_REQUEST_HANDLER)
})
})

describe('type safety', () => {
it('should return a string in all cases', () => {
// Arrange
const testInputs = [undefined, '', 'handler', 'builtin:notFound']

// Act & Assert
testInputs.forEach(input => {
const result = requestHandlerLabel(input)
expect(typeof result).toBe('string')
})
})

it('should always return UNNAMED_REQUEST_HANDLER or the provided string', () => {
// Arrange
const validHandler = 'myHandler'
const unnamedHandler = undefined

// Act
const resultValid = requestHandlerLabel(validHandler)
const resultUnnamed = requestHandlerLabel(unnamedHandler)

// Assert
expect(resultValid === validHandler || resultValid === UNNAMED_REQUEST_HANDLER).toBe(true)
expect(resultUnnamed === UNNAMED_REQUEST_HANDLER).toBe(true)

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

The assertion collapses the comparison to a boolean before passing it to expect, so a failure reports only 'expected true, received false' with no indication of the actual returned value. The disjunction also makes it a near-tautology: it passes whenever either branch is taken, so it cannot detect the function returning the fallback for a valid handler name.

Action: Replace with direct assertions on each case: expect(resultValid).toBe(validHandler) and expect(resultUnnamed).toBe(UNNAMED_REQUEST_HANDLER).

To dismiss: /dk-review dismiss a48f7d13-6e0b-4f92-b3c5-70a9e2d16b48 [reason]

})
})

describe('long handler names', () => {
it('should handle very long handler names', () => {
// Arrange
const longName = 'a'.repeat(1000)

// Act
const result = requestHandlerLabel(longName)

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

describe('numeric and special string inputs', () => {
it('should handle numeric strings', () => {
// Arrange
const numericString = '12345'

// Act
const result = requestHandlerLabel(numericString)

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

it('should handle zero as a string', () => {
// Arrange
const zeroString = '0'

// Act
const result = requestHandlerLabel(zeroString)

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

it('should handle the string "false"', () => {
// Arrange
const falseString = 'false'

// Act
const result = requestHandlerLabel(falseString)

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

it('should handle the string "null"', () => {
// Arrange
const nullString = 'null'

// Act
const result = requestHandlerLabel(nullString)

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