test(csharp): de-flake FeatureFlagCache single-external-call (assert dedup) - #630
test(csharp): de-flake FeatureFlagCache single-external-call (assert dedup)#630eric-wang-1990 wants to merge 1 commit into
Conversation
…dedup) TestFeatureFlagCache_SingleExternalCallAcrossConnections asserted EXACTLY one external feature-flag fetch across 5 connections. That is too strict: healthy fetches get a 15-min sliding TTL (all 5 share one fetch → 1), but if the FIRST fetch hits a transient failure/timeout it is cached with a short 60s NEGATIVE TTL (FeatureFlagContext.DefaultNegativeTtl) BY DESIGN, so a later connection re-fetches → 2. The cache is still deduping (3-4 of 5 served from cache); the exact-1 assertion turned that designed-in retry into a flake (observed Expected 1 / Actual 2, once, in a merge-queue run of #627). Assert DEDUP instead of exact-1: fetchCount >= 1, < connectionCount, and <= 2 (one initial fetch, plus at most one transient negative-cache re-fetch). This still fails loudly if the cache genuinely stops deduping (fetchCount ~= connectionCount) while tolerating a single environmental hiccup. Co-authored-by: Isaac
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a sound, test-only de-flake that relaxes an over-strict == 1 assertion to a dedup check (>= 1 && < connectionCount && <= 2), with clear rationale in the comment. One low: the test's <summary> docstring still claims "exactly one" external fetch and now contradicts the new assertion.
|
|
||
| // Assert - exactly one external call; the rest served from the shared cache. | ||
| Assert.Equal(1, fetchCount); | ||
| // Assert - the shared cache DEDUPS: far fewer external fetches than connections. |
There was a problem hiding this comment.
🔵 Low — The <summary> docstring for this test still asserts the old, now-removed contract: it says opening multiple connections "results in exactly one actual connector-service fetch; the rest are served from the cache." The body of the PR deliberately relaxes that to tolerate a second fetch under the negative-cache TTL. The docstring now contradicts the assertion (fetchCount <= 2) directly below it. Consider updating it to describe dedup ("at most 2 fetches") so the stated contract matches the code.
The test method name TestFeatureFlagCache_SingleExternalCallAcrossConnections is similarly stale, but renaming it is optional churn; the docstring is the clearer fix.
(Anchored to the nearest changed line — see the description for the exact location.)
1 participant
What
De-flakes
FeatureFlagCacheE2ETest.TestFeatureFlagCache_SingleExternalCallAcrossConnectionsby asserting the cache dedups rather than demanding exactly 1 external fetch.Why
The test opens 5 connections to the same host and asserted
Assert.Equal(1, fetchCount). That's too strict:1(the usual pass).FeatureFlagContext.DefaultNegativeTtl) by design, so a later connection re-fetches →2.The cache is still deduping in both cases (3–4 of 5 served from cache); the negative-cache retry is a feature. The exact-1 assertion turned that designed-in retry into a flake — observed
Expected: 1 / Actual: 2in a merge-queue run of #627 (job 92235923069). History confirms it's low-rate: this test passed the other recent runs and failed once,Actualalways 2 (never higher).Fix
Assert dedup:
fetchCount >= 1 && fetchCount < connectionCount && fetchCount <= 2— one initial fetch plus at most one transient negative-cache re-fetch. Still fails loudly if the cache genuinely stops deduping (fetchCount ≈ connectionCount), while tolerating a single environmental hiccup.This pull request and its description were written by Isaac.