State what the refresh-slot routine-band case establishes, in the present tense - #3181
Conversation
…sent tense The comment above the InsideSlot case narrated a failure that no longer exists: it said the recorded ceiling logs Warning rather than Debug and that the assertion below it fails. Both are false on the current grid, and the assertion passes. The ceiling is 896 s against a 1,260 s slot and a 1,050 s watch line, so ClassifyRefreshSlotHeadroom bands it InsideSlot and LogHeaviestRefreshSlotHeadroom takes the Debug arm. The replacement states that, and keeps the reason the case is fed the grid's own sizing figure rather than a lower literal: both sides are read from TimescaleSupport, so the case goes red if a census raises the ceiling or a re-derivation narrows the slot until the product would call its own sizing figure a warning. The assertion and the value it is fed are unchanged.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed. This is a genuinely comment-only change to Correctness of the new comment's claims — verified against
So the replacement comment is factually accurate and no longer narrates a since-resolved failure (the old comment's claim that this assertion FAILS is false against current values, as the PR description documents). This isn't a T-SQL change and there's no Lite counterpart to this file (TimescaleDB continuous-aggregate refresh scheduling is Darling/Postgres-specific; Lite has no equivalent), so parity isn't a concern here. Left one inline nit: the new comment doesn't cite an issue number, which is otherwise consistent practice in this file and is called out explicitly in CONTRIBUTING.md's comment-style guidance. No correctness, security, or performance concerns — this is test-file documentation only, nothing here touches runtime code paths. |
|
Reviewed. This is a comment-only change in Verified the new comment's factual claims against the current
The replacement is accurate, present-tense, and no longer narrates a historical failure (consistent with the repo's comment convention). No correctness, security, or performance concerns — nothing to flag. |
1 participant
Comment-only change in
TimescaleSupportTests.cs. The assertion and the value it is fed are untouched —git diffis nine lines added, seven removed, all inside one block comment.What was wrong
The comment above the routine-band case in
TheRefreshSlotLogLine_IsLeveledByBand_AndSaysNothingWithoutAReadingnarrated a failure:Both factual claims are false on the current grid, and the assertion below it passes. That also makes the comment a history-of-a-change note rather than a statement of what the code does, which is what the repo's comment convention rules out.
What is true, measured rather than reasoned
Evaluated by compiling
PerformanceMonitor.Darling.Storage(plainnet10.0) and reading the real derivation chain through reflection, rather than hand-computing it:HeaviestHourlyRefreshObservedCeilingSecondsHeaviestRefreshWindowMinutesRefreshPhaseSlotSecondsRefreshSlotWarningSecondsClassifyRefreshSlotHeadroom(896)InsideSlotClassifyRefreshSlotHeadroom(1050)ApproachingSlotClassifyRefreshSlotHeadroom(1260)SlotExceededInsideSlotfalls toLogHeaviestRefreshSlotHeadroom'sdefault:arm, which isLogDebug. So the case establishes that the grid's own sizing figure is a routine reading — 71.1% of the slot, 154 s below the watch line.#3178 is what changed it: the slot went to 1,260 s and the watch line to 1,050 s, and #3178's own red-tests table already records the outcome ("the line at the ceiling is
Debug:").What the replacement keeps
The stale comment's reasoning was right and is preserved: re-pointing the case at an arbitrary lower literal would keep the level table covered while dropping the claim worth holding — that the figure the grid is sized around is routine, not a warning. The replacement states that in the present tense and adds why it stays true without maintenance: both sides are read from
TimescaleSupportrather than written as literals here, so the ceiling and the watch line can only meet through a real change (a census that raises the ceiling, a re-derivation that narrows the slot), and when they do this case goes red rather than the product quietly calling its own sizing figure a warning.Verification
Darling.Teststargetsnet10.0-windowsand cannot run on macOS, so the shippedTimescaleSupportTests.cswas compiled unmodified — with its real helpersCapturingTestLogger,LiveStoreCleanup,LiveCleanupBatch,IlCallSiteScanner— into anet10.0xunit.v3 harness namedDarling.TestssoInternalsVisibleTostill applies.48 total, 0 failed, 0 errors, 15 skipped, 0 not run. Every skip is live-Postgres gated (
DARLING_TEST_PG). The target case is absent from all 15[SKIP]lines, so it ran rather than being counted as covered.Red-proofed. With the fed value mutated from
HeaviestHourlyRefreshObservedCeilingSecondstoRefreshSlotWarningSeconds, the case fails withAssert.StartsWith() Failure: String start does not match / String: "Warning: TimescaleDB: query_store_stats_interval_h"···— so the Debug assertion discriminates and does not pass vacuously. Mutation confirmed present in the tree bygit diff --numstatplus a content check before running, and confirmed restored by content afterwards, with CRLF endings intact (3,819 CRLF, 0 bare LF, no BOM).dotnet build Darling/Darling.Tests/Darling.Tests.csproj -p:EnableWindowsTargeting=truesucceeds, 0 errors.CHANGELOG entry text (not committed)
Under
[Unreleased]→Fixed:InsideSlotand logs Debug, and the comment now states what the case establishes instead of narrating a repaired failure.