documentation cleanse, sprints work and planning. remaining non EF DAL migration to EF
This commit is contained in:
master
@@ -0,0 +1,17 @@
|
||||
{
|
||||
"feature": "graph-overlay-system",
|
||||
"approved": true,
|
||||
"reason": "Confirmed: MeterListener cross-contamination via meter name matching. The OverlayCacheCounters_RecordHitsAndMisses test subscribes to instruments by meter NAME ('StellaOps.Graph.Api') at line 67, while BudgetDeniedCounter_IncrementsOnEdgeBudgetExceeded correctly subscribes by meter INSTANCE reference (instrument.Meter == metrics.Meter) at line 25. When run in parallel with QueryServiceTests.OverlayMerge_IncludesExplainTrace, which creates an undisposed GraphMetrics and triggers 2 overlay cache misses (2 nodes with IncludeOverlays=true), the listener picks up those 2 extra miss events + 1 from its own test = 3 total (matches the observed 'Expected: 1, Actual: 3'). Verified experimentally: test passes in isolation (-class MetricsTests, 2/2 pass), fails with QueryServiceTests (-class MetricsTests -class QueryServiceTests, 1 fail with exact Expected:1 Actual:3), and passes with -maxThreads 1 (serial execution, 0 MetricsTests failures).",
|
||||
"revisedRootCause": "The OverlayCacheCounters_RecordHitsAndMisses test's MeterListener subscribes by meter name string ('StellaOps.Graph.Api') instead of by meter instance reference, causing it to receive overlay cache miss events from undisposed GraphMetrics instances in other test classes (specifically QueryServiceTests.OverlayMerge_IncludesExplainTrace which creates an undisposed GraphMetrics and triggers 2 cache misses for 2 nodes). The BudgetDeniedCounter test in the same class is not affected because it correctly filters by instance reference. This is a parallelism + resource disposal issue, not just meter naming.",
|
||||
"revisedCategory": "test_gap",
|
||||
"revisedAffectedFiles": [
|
||||
"src/Graph/__Tests/StellaOps.Graph.Api.Tests/MetricsTests.cs",
|
||||
"src/Graph/__Tests/StellaOps.Graph.Api.Tests/QueryServiceTests.cs"
|
||||
],
|
||||
"blastRadius": "low",
|
||||
"regressionRisk": "none - fix is limited to test instrumentation subscription, not production code",
|
||||
"revisedSuggestedFix": "Change line 67 in MetricsTests.cs from 'instrument.Meter.Name == \"StellaOps.Graph.Api\"' to 'instrument.Meter == metrics.Meter' (matching the pattern already used in BudgetDeniedCounter test at line 25). This is the simplest fix — it uses instance-level filtering to isolate from other tests' meters. Additionally, QueryServiceTests.cs line 78 and line 117 should add 'using' to dispose GraphMetrics instances, preventing leaked meters from affecting other tests. The triage's suggested fix of unique meter names per test would also work but is unnecessarily invasive — it requires modifying production code (GraphMetrics constructor) when the real issue is purely in test listener subscription.",
|
||||
"additionalContext": "15 other test files in the project create GraphMetrics() without 'using' disposal. While fixing the listener subscription in MetricsTests.cs is sufficient to fix THIS test, the broader pattern of undisposed meters is a latent test hygiene issue that could cause similar problems if other tests add MeterListener-based assertions.",
|
||||
"confidence": 0.98,
|
||||
"verificationMethod": "Ran tests in 3 configurations: (1) MetricsTests alone: 2/2 pass; (2) MetricsTests + QueryServiceTests: OverlayCacheCounters fails with Expected:1 Actual:3; (3) All tests with -maxThreads 1: MetricsTests passes (only EdgeMetadataServiceTests fail, unrelated). This conclusively proves parallel cross-contamination via name-based MeterListener subscription."
|
||||
}
|
||||
@@ -0,0 +1,16 @@
|
||||
{
|
||||
"feature": "graph-overlay-system",
|
||||
"module": "graph",
|
||||
"filesModified": [
|
||||
"src/Graph/__Tests/StellaOps.Graph.Api.Tests/MetricsTests.cs",
|
||||
"src/Graph/__Tests/StellaOps.Graph.Api.Tests/QueryServiceTests.cs"
|
||||
],
|
||||
"testsAdded": [],
|
||||
"description": "Aligned overlay cache counter listener to subscribe by GraphMetrics meter instance instead of meter name and updated QueryServiceTests to dispose GraphMetrics instances with using to prevent MeterListener cross-test contamination.",
|
||||
"buildVerified": true,
|
||||
"testResults": {
|
||||
"passed": 52,
|
||||
"failed": 0
|
||||
},
|
||||
"notes": "dotnet build and dotnet test both pass for StellaOps.Graph.Api.Tests after the test-only fixes."
|
||||
}
|
||||
@@ -0,0 +1,32 @@
|
||||
{
|
||||
"previousFailures": [
|
||||
{
|
||||
"tier": 1,
|
||||
"reason": "1 MetricsTests.OverlayCacheCounters failed due to MeterListener cross-contamination between tests — name-based meter filtering picked up instruments from other tests' GraphMetrics instances"
|
||||
}
|
||||
],
|
||||
"retestResults": [
|
||||
{
|
||||
"tier": 1,
|
||||
"result": "pass",
|
||||
"evidence": "dotnet build succeeded with 0 errors, 0 warnings. dotnet test Graph.Api.Tests: 52 passed, 0 failed, 0 skipped. MetricsTests: 2/2 passed (OverlayCacheCounters now passes with instance-based meter filtering). QueryServiceTests: 3/3 passed (using statements for GraphMetrics confirmed no regression)."
|
||||
}
|
||||
],
|
||||
"regressionCheck": {
|
||||
"testsRun": 108,
|
||||
"testsPassed": 108,
|
||||
"testsFailed": 0,
|
||||
"newTestsRun": 2,
|
||||
"newTestsPassed": 2,
|
||||
"details": {
|
||||
"Graph.Api.Tests": { "total": 52, "passed": 52, "failed": 0 },
|
||||
"Graph.Core.Tests": { "total": 19, "passed": 19, "failed": 0 },
|
||||
"Graph.Indexer.Tests": { "total": 37, "passed": 37, "failed": 0 },
|
||||
"Graph.Indexer.Persistence.Tests": { "total": 17, "passed": 0, "failed": 17, "skipped": true, "skipReason": "Requires Docker/PostgreSQL Testcontainers — environment not available" }
|
||||
}
|
||||
},
|
||||
"verdict": "pass",
|
||||
"failureDetails": null,
|
||||
"notes": "MetricsTests.OverlayCacheCounters now passes with instance-based meter filtering fix. QueryServiceTests (3/3) pass with GraphMetrics using statements — no regression. All 108 non-persistence tests pass. Persistence tests (17) skipped due to Docker/PostgreSQL unavailability — pre-existing, not a regression.",
|
||||
"retestDateUtc": "2026-02-09T21:43:00Z"
|
||||
}
|
||||
@@ -0,0 +1,20 @@
|
||||
{
|
||||
"feature": "graph-overlay-system",
|
||||
"module": "graph",
|
||||
"rootCause": "MeterListener in test picks up overlay cache miss events from previous test's undisposed Meter instance, causing accumulated miss count of 3 instead of expected 1",
|
||||
"category": "test_gap",
|
||||
"affectedFiles": [
|
||||
"src/Graph/__Tests/StellaOps.Graph.Api.Tests/MetricsTests.cs"
|
||||
],
|
||||
"confidence": 0.75,
|
||||
"details": {
|
||||
"failedTests": [
|
||||
{
|
||||
"testName": "MetricsTests.OverlayCacheCounters_RecordHitsAndMisses",
|
||||
"rootCause": "MeterListener subscribes to all Meter instances with name 'StellaOps.Graph.Api' globally, not just the current test's metrics instance. The first test (BudgetDeniedCounter_IncrementsOnEdgeBudgetExceeded) creates a GraphMetrics instance with a Meter of the same name. Even though the first test uses 'using var metrics', the Meter disposal may not complete before the second test's listener starts, causing the listener to aggregate events from both tests. The test expects 1 cache miss (first query) and 1 cache hit (second query), but gets 3 misses total, suggesting cross-test contamination.",
|
||||
"suggestedFix": "Isolate each test's meter by using unique meter names (e.g., include test name or GUID in meter name), OR use a test-specific MeterListener that filters by meter instance rather than meter name, OR ensure meters are fully disposed before starting the next test's listener (e.g., add explicit disposal wait or use AsyncTestSyncContext), OR restructure the test to avoid shared meter naming by creating separate test classes for each metrics test"
|
||||
}
|
||||
]
|
||||
},
|
||||
"suggestedFix": "Use unique meter names per test to prevent MeterListener cross-contamination. Change GraphMetrics constructor to accept an optional meter name suffix, and modify the test to pass a unique identifier (e.g., test method name or GUID) when creating GraphMetrics instances in tests."
|
||||
}
|
||||
Reference in New Issue
Block a user