[DK TestAI] Add unit tests for requestHandlerLabel - #689
Conversation
|
0 New Issues
0 Fixed Issues
0 Accepted Issues![dk-pr-review[bot]](https://avatars.githubusercontent.com/in/4406719?s=60&v=4){kind=small}
There was a problem hiding this comment.
DK Review — Audit Summary
Verdict: ❌ BLOCKED
| Severity | Count |
|---|---|
| BLOCK | 1 |
| RESTRICT | 0 |
| SUGGEST | 9 |
Scenarios evaluated: general-review, quality-ratchet
Scenarios skipped: dependency-governance (no matching files), pipeline-config (no matching files), agent-skills-review (no matching files)
📋 Findings (10)
BLOCK
- [Functional.Interface]
src/service/metrics/requestHandlerLabel.test.ts:1— The test file importsUNNAMED_REQUEST_HANDLERandrequestHandlerLabelfrom './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.
→ Includesrc/service/metrics/requestHandlerLabel.ts(exportingUNNAMED_REQUEST_HANDLERandrequestHandlerLabel) in this PR, or point the import at the module where the helper actually lives.
SUGGEST
- [general.broken-references]
src/service/metrics/requestHandlerLabel.test.ts:1— The test importsUNNAMED_REQUEST_HANDLERandrequestHandlerLabelfrom './requestHandlerLabel', butsrc/service/metrics/requestHandlerLabel.tsdoes 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.
→ Confirm thatsrc/service/metrics/requestHandlerLabel.tsis included in this PR (and thatrequestHandlerLabeldeclares its parameter as optional, since line 91 calls it with no argument). Runyarn testlocally to verify the module resolves. - [Evolvability.Textual]
src/service/metrics/requestHandlerLabel.test.ts:76— The describe block is named 'edge cases - undefined and null' but contains no test for anullinput — onlyundefinedand the omitted-argument case. The block name promises coverage the suite does not provide, which is misleading for future maintainers auditing coverage.
→ Either add arequestHandlerLabel(null as any)case asserting the fallback (this module is consumed by plain-JS callers via the publishedlib/, wherenullis reachable), or rename the block to 'edge cases - undefined'. - [Functional.Check]
src/service/metrics/requestHandlerLabel.test.ts:134— 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.
→ 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'). - [Evolvability.SolutionApproach]
src/service/metrics/requestHandlerLabel.test.ts:214— The assertion collapses the comparison to a boolean before passing it toexpect, 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.
→ Replace with direct assertions on each case:expect(resultValid).toBe(validHandler)andexpect(resultUnnamed).toBe(UNNAMED_REQUEST_HANDLER). - [Evolvability.Organizational]
src/service/metrics/requestHandlerLabel.test.ts:149— 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.
→ 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. - [Evolvability.Organizational]
src/service/metrics/requestHandlerLabel.test.ts:1— The file is placed directly insrc/service/metrics/while the existing test in this module lives insrc/service/metrics/__tests__/clusterMetricsAggregator.test.ts. Jest'stestRegexpicks it up either way, so this is not a functional break, but the split location makes module tests harder to locate.
→ Move the file tosrc/service/metrics/__tests__/requestHandlerLabel.test.tsand adjust the import to../requestHandlerLabel, matching the sibling test. - [Evolvability.Organizational]
src/service/metrics/requestHandlerLabel.test.ts:1— The test is placed directly insrc/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 jesttestPathIgnorePatterns/build-exclusion config error-prone, and a stray test file undersrc/can be picked up by the TypeScript build output.
→ Move the file tosrc/service/metrics/__tests__/requestHandlerLabel.test.tsand adjust the import to '../requestHandlerLabel'. - [Functional.Check]
src/service/metrics/requestHandlerLabel.test.ts:142— 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.
→ Decide the intended contract explicitly: either trim the input inrequestHandlerLabeland fall back toUNNAMED_REQUEST_HANDLERfor whitespace-only names (updating these two tests), or add a comment in the test explaining why whitespace-only names are deliberately preserved. - [quality.new-logic-enforcement]
src/service/metrics/requestHandlerLabel.test.ts:1— 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.
→ Land the implementation together with these tests and confirm the coverage run passes and reports a non-negative delta against the baseline before merging.
DK Review v1.0.0 | To dismiss a finding: reply /dk-review dismiss <finding-id> [reason]
| @@ -0,0 +1,279 @@ | |||
| import { UNNAMED_REQUEST_HANDLER, requestHandlerLabel } from './requestHandlerLabel' | |||
|
|
|||
There was a problem hiding this comment.
[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]
| }) | ||
|
|
||
| describe('edge cases - undefined and null', () => { | ||
| it('should return UNNAMED_REQUEST_HANDLER when requestHandlerName is undefined', () => { |
There was a problem hiding this comment.
[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]
|
|
||
| // Assert | ||
| expect(result).toBe(' ') | ||
| }) |
There was a problem hiding this comment.
[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]
|
|
||
| // Assert | ||
| expect(resultValid === validHandler || resultValid === UNNAMED_REQUEST_HANDLER).toBe(true) | ||
| expect(resultUnnamed === UNNAMED_REQUEST_HANDLER).toBe(true) |
There was a problem hiding this comment.
[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('consistency with aggregation expectations', () => { | ||
| it('should return consistent string for undefined across multiple calls', () => { |
There was a problem hiding this comment.
[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]
| @@ -0,0 +1,279 @@ | |||
| import { UNNAMED_REQUEST_HANDLER, requestHandlerLabel } from './requestHandlerLabel' | |||
|
|
|||
There was a problem hiding this comment.
[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]
| @@ -0,0 +1,279 @@ | |||
| import { UNNAMED_REQUEST_HANDLER, requestHandlerLabel } from './requestHandlerLabel' | |||
|
|
|||
There was a problem hiding this comment.
[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]
| @@ -0,0 +1,279 @@ | |||
| import { UNNAMED_REQUEST_HANDLER, requestHandlerLabel } from './requestHandlerLabel' | |||
|
|
|||
There was a problem hiding this comment.
[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]
|
|
||
| // Act | ||
| const result = requestHandlerLabel(singleSpace) | ||
|
|
There was a problem hiding this comment.
[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]
| @@ -0,0 +1,279 @@ | |||
| import { UNNAMED_REQUEST_HANDLER, requestHandlerLabel } from './requestHandlerLabel' | |||
|
|
|||
There was a problem hiding this comment.
[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]
Generated Unit Tests for requestHandlerLabel.ts
Summary
Generated and validated 1 unit test that passed the QA filter pipeline.
Generated Test Files
src/service/metrics/requestHandlerLabel.test.tsTest Execution
Skipped (SKIP_FILTER_STAGES=execution)
Coverage
PR baseline (measured on base branch before this test PR): N/A
Threshold: >=10.0%
Skipped (SKIP_FILTER_STAGES=coverage)
Flakiness
Skipped (SKIP_FILTER_STAGES=flakiness)
Mutation Testing
Threshold: >=80.0%
Skipped (SKIP_FILTER_STAGES=mutation)
Filter Results
Next Steps
Generated by DK TestAI