test: fix flaky unit tests that compare time-dependent fields - #2800
Open
kimurakazuhiro-c wants to merge 9 commits into
Open
test: fix flaky unit tests that compare time-dependent fields#2800kimurakazuhiro-c wants to merge 9 commits into
kimurakazuhiro-c wants to merge 9 commits into
Conversation
…edAt NewAutoOpsRule stamps CreatedAt and UpdatedAt at call time, so comparing them with values built before the call fails whenever the second boundary is crossed. Assert both are close to now instead, and check that UpdatedAt is not older than CreatedAt because AddDatetimeClause and AddOpsEventRateClause re-stamp UpdatedAt inside the constructor. Refs #2793 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ChangeRulesOrder stamps UpdatedAt at call time, so comparing it with a value built before the call fails whenever the second boundary is crossed. Replace the expected timestamp with a flag describing the expected behaviour: the success case asserts UpdatedAt is close to now, and the error cases assert it is left untouched. Refs #2793 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
UpdateSubscription stamps UpdatedAt at call time, so the whole-struct comparison fails whenever the second boundary is crossed between building the expectation and running the call. Assert the actual UpdatedAt is close to now, then copy it into the expectation so every other field is still compared exactly. Refs #2793 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fixture stamped UpdatedAt with time.Now() and the tests compared it with the value NewAccountV2 stamps at call time, so they failed whenever the second boundary was crossed. UpdatedAt is not an argument of NewAccountV2, so the fixture field never reached the account under test and the comparison only passed by coincidence. Drop the field from the fixtures and assert the account under test has UpdatedAt close to now, which is what AddSearchFilter and the other search filter operations actually guarantee. Refs #2793 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…oppedAt Update stamps StartAt and StoppedAt at call time, and the test captured its own now at the top of the function, so the comparison failed whenever the second boundary was crossed. The flakiness window was the whole test function rather than a single call. Describe the expected behaviour with flags instead: the cases that trigger a status transition assert the stamped field is close to now, and the remaining cases keep the exact comparison that verifies the field is left untouched. StopAt is not stamped by Update, so it is unchanged. Refs #2793 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Server-stamped timestamps were compared against a time captured in the test setup, so the assertions failed whenever the second boundary was crossed. Assert those fields against the current time with a tolerance, then copy the actual value into expected for the full-struct comparison. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
In account_test.go the InDelta assertions could not fail: NewAccountV2 and the setup AddSearchFilter calls already stamp UpdatedAt with the current time, so removing the stamp from the mutator under test still passed. Backdate UpdatedAt before the call and branch on whether the mutator is expected to stamp it, which also covers the error paths leaving the field untouched. In TestChangeRulesOrder the expected value was captured once before the loop over a shared, mutated feature, so the assertion only held because the stamping case happened to be last. Read the previous value inside the loop instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three patterns in TestAddSearchFilter have at least one expected filter, so `len(p.expectedFilters) > 0` was always true and the else branch was never exercised. Drop the branch and keep the plain InDelta assertion; backdateUpdatedAt is still called so the assertion stays falsifiable if AddSearchFilter ever stops stamping UpdatedAt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kimurakazuhiro-c
marked this pull request as ready for review
September 3, 2026 02:14
kimurakazuhiro-c
requested review from
cre8ivejp,
hvn2k1 and
t-kikuc
as code owners
September 3, 2026 02:14
There was a problem hiding this comment.
🟢 Approval recommended
The test-only changes resolve flaky timestamp assertions without weakening other checks.
Pull request overview
Stabilizes flaky unit tests that compare server-generated timestamps. No production code is changed.
Changes:
- Uses bounded assertions for generated timestamps.
- Preserves exact checks for unchanged timestamps and other fields.
- Normalizes timestamps before whole-structure comparisons.
File summaries
| File | Description |
|---|---|
pkg/subscription/domain/subscription_test.go |
Stabilizes UpdatedAt assertions. |
pkg/feature/domain/feature_test.go |
Handles conditional UpdatedAt stamping. |
pkg/experiment/domain/experiment_test.go |
Stabilizes start and stop timestamp checks. |
pkg/autoops/domain/auto_ops_rule_test.go |
Validates generated timestamps and their ordering. |
pkg/api/api/api_test.go |
Stabilizes REST evaluation timestamps. |
pkg/api/api/api_grpc_test.go |
Stabilizes gRPC evaluation and request timestamps. |
pkg/account/domain/account_test.go |
Verifies successful updates and error-path timestamp preservation. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ent-tests # Conflicts: # pkg/api/api/api_grpc_test.go # pkg/api/api/api_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2793, Related #1023
What this PR does
Removes the flakiness from unit tests that compared server-stamped time fields (
CreatedAt/UpdatedAt/RequestedAt/StartAt/StoppedAt) against atime.Now()captured in the test setup. Covers all locations listed in #2793:autoops,feature,subscription,account,experiment, andapi.Background / Why this PR is needed
These tests built their expected value with
time.Now().Unix()before invoking the code under test, and the code under test stamps its owntime.Now().Unix(). The two values are equal only when no second boundary is crossed in between, so the tests failed intermittently in CI — the original report was #1023 forTestGrpcGetEvaluationsValidation, andpkg/api/api/api_test.goeven carried a// FIXME: This is a flaky test. CreateAt may not be equal ocasionally.comment acknowledging the problem — the assertion was left active, so the test kept failing intermittently.For
TestUpdateExperimentthe window was worse than a single call:nowwas captured once at the top of the function and reused across every table case, so the whole test function was the race window.Points
autoops,feature,account,experiment): the exact comparison is replaced withassert.InDelta(t, time.Now().Unix(), actual, 5).subscription,api): the stamped field is asserted withInDeltafirst, then copied intoexpectedso every other field is still compared exactly. This keeps the strictassert.Equalon the full struct rather than weakening it.TestChangeRulesOrderandTestUpdateExperimentexpress this with an explicit flag (expectUpdatedAtNow,expectStartAtNow,expectStoppedAtNow) so the error paths still verify the timestamp is left untouched — a behaviour the oldexpectedUpdateAt: f.UpdatedAtform was checking, and it is preserved.assert.GreaterOrEqual(t, aor.UpdatedAt, aor.CreatedAt)is added inTestNewAutoOpsRulebecauseAddDatetimeClause/AddOpsEventRateClausere-stampUpdatedAtinside the constructor, so the ordering is a real invariant worth asserting.pkg/account/domain/account_test.gothe fixtureUpdatedAtwas never actually reaching the account under test —UpdatedAtis not an argument ofNewAccountV2— so the old assertion only passed by coincidence. The field is dropped from the fixtures.// FIXME: This is a flaky testcomment inTestGetEvaluationsValidationis resolved at its source:CreatedAtis dropped from theemptyUserEvaluationsForRESTfixture and asserted withassertEvaluationsCreatedAtNowForREST, so the full-structassert.Equalstays strict and is no longer time-dependent. The comment is removed.