Skip to content

Commit b879f83

Browse files
committed
fix: remove dead code from createPerformanceCard, fix CSV zero-value bug, add differential tests
- Remove unreachable avgResponseTime/lastExecutionTime branches from createPerformanceCard (its metrics list only has memory/CPU/disk/network keys) - Fix CSV export treating avg_response_time=0 as N/A by using != null instead of truthiness check, consistent with table rendering - Fix trailing whitespace in updateTableRows - Update modularized test assertion for ×1000 conversion (150 → 150000) - Add JS tests for createMetricsCard: seconds→ms conversion, ISO datetime formatting, null handling, zero-value preservation - Add Python test verifying build_top_performers preserves 0.0 values Closes #3793, Closes #3580 Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
1 parent e1546ff commit b879f83

3 files changed

Lines changed: 47 additions & 17 deletions

File tree

mcpgateway/admin_ui/metrics.js

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1494,7 +1494,7 @@ export const updateTableRows = function (
14941494
avgTimeCell.className =
14951495
"px-6 py-4 whitespace-nowrap text-sm text-gray-500 dark:text-gray-300 sm:px-6 sm:py-4";
14961496
const avgTime = item.avg_response_time ?? item.avgResponseTime;
1497-
avgTimeCell.textContent = avgTime != null ? `${Math.round(Number(avgTime) * 1000)}ms` : "N/A" ;
1497+
avgTimeCell.textContent = avgTime != null ? `${Math.round(Number(avgTime) * 1000)}ms` : "N/A";
14981498
row.appendChild(avgTimeCell);
14991499

15001500
// Success Rate
@@ -1554,7 +1554,7 @@ item.execution_count ||
15541554
item.executions ||
15551555
0,
15561556
),
1557-
item.avg_response_time ?? item.avgResponseTime
1557+
(item.avg_response_time ?? item.avgResponseTime) != null
15581558
? `${Math.round((item.avg_response_time ?? item.avgResponseTime) * 1000)}ms`
15591559
: "N/A",
15601560
`${calculateSuccessRate(item)}%`,
@@ -1659,20 +1659,7 @@ export const createPerformanceCard = function (performanceData) {
16591659

16601660
const valueSpan = document.createElement("span");
16611661
valueSpan.className = "font-medium dark:text-gray-200";
1662-
let displayValue;
1663-
if (value === "N/A") {
1664-
displayValue = "N/A";
1665-
} else if (metric.key === "avgResponseTime") {
1666-
const ms = Number(value);
1667-
displayValue = isNaN(ms) ? "N/A" : `${(ms * 1000).toFixed(1)} ms`;
1668-
} else if (metric.key === "lastExecutionTime") {
1669-
displayValue = typeof value === "string" && value.includes("T")
1670-
? value.slice(0, 16).replace("T", " ")
1671-
: String(value);
1672-
} else {
1673-
displayValue = String(value);
1674-
}
1675-
valueSpan.textContent = displayValue;
1662+
valueSpan.textContent = value === "N/A" ? "N/A" : String(value);
16761663

16771664
metricRow.appendChild(label);
16781665
metricRow.appendChild(valueSpan);

tests/unit/js/metrics.test.js

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -609,7 +609,7 @@ describe("extractKPIData", () => {
609609
};
610610

611611
const kpi = extractKPIData(data);
612-
expect(kpi.avgResponseTime).toBe(150); // Weighted average
612+
expect(kpi.avgResponseTime).toBe(150000); // Weighted average (seconds × 1000 → ms)
613613
});
614614

615615
test("handles null avgResponseTime", () => {
@@ -825,6 +825,7 @@ describe("createPerformanceCard", () => {
825825
expect(card).toBeInstanceOf(HTMLElement);
826826
consoleError.mockRestore();
827827
});
828+
828829
});
829830

830831
describe("createRecentActivitySection", () => {
@@ -904,6 +905,31 @@ describe("createMetricsCard", () => {
904905
const card = createMetricsCard("Prompts", {});
905906
expect(card.textContent).toContain("N/A");
906907
});
908+
909+
test("converts avgResponseTime from seconds to milliseconds", () => {
910+
const metrics = { avgResponseTime: 1.5 };
911+
const card = createMetricsCard("Tools", metrics);
912+
expect(card.textContent).toContain("1500.0 ms");
913+
});
914+
915+
test("formats lastExecutionTime as human-readable date", () => {
916+
const metrics = { lastExecutionTime: "2026-03-13T06:00:00.000000" };
917+
const card = createMetricsCard("Tools", metrics);
918+
expect(card.textContent).toContain("2026-03-13 06:00");
919+
});
920+
921+
test("displays N/A for null avgResponseTime", () => {
922+
const metrics = { avgResponseTime: null };
923+
const card = createMetricsCard("Tools", metrics);
924+
// avgResponseTime null → value resolves to "N/A" via ?? chain
925+
expect(card.textContent).toContain("N/A");
926+
});
927+
928+
test("preserves zero avgResponseTime", () => {
929+
const metrics = { avgResponseTime: 0 };
930+
const card = createMetricsCard("Tools", metrics);
931+
expect(card.textContent).toContain("0.0 ms");
932+
});
907933
});
908934

909935
// ===================================================================

tests/unit/mcpgateway/utils/test_metrics_common.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,3 +44,20 @@ def test_build_top_performers_handles_missing_values():
4444
assert performers[0].avg_response_time is None
4545
assert performers[0].success_rate is None
4646
assert performers[0].last_execution is None
47+
48+
49+
def test_build_top_performers_preserves_zero_values():
50+
"""Zero avg_response_time and success_rate must not be coerced to None."""
51+
result = SimpleNamespace(
52+
id=3,
53+
name="zero-tool",
54+
execution_count=10,
55+
avg_response_time=0.0,
56+
success_rate=0.0,
57+
last_execution=None,
58+
)
59+
60+
performers = build_top_performers([result])
61+
62+
assert performers[0].avg_response_time == 0.0
63+
assert performers[0].success_rate == 0.0

0 commit comments

Comments
 (0)