test: replace t.Skip with t.Fatalf for replication-factor assertions#104
Merged
test: replace t.Skip with t.Fatalf for replication-factor assertions#104
Conversation
Precondition failures in distributed cache tests (read-repair, stale quorum, and phase-1 basic quorum) were silently swallowed via t.Skip, masking genuine test-environment regressions as benign skips. - \`TestDistMemoryReadRepair\`: Skip "replication factor <2" → Fatalf with the actual owner count; RF=2 with 2 registered nodes must always yield ≥2 owners. - \`TestDistMemoryStaleQuorum\`: Skip "replication factor !=3" → Fatalf; RF=3 with 3 registered nodes must yield exactly 3 owners. - \`TestDistPhase1BasicQuorum\`: Remove all skip-on-miss / skip-on-mismatch branches (placeholders from before hint-replay was wired). Now strictly asserts that nodeC receives and holds the correct replicated value within the 3 s deadline, since hinted handoff replay is fully operational (see TestHintedHandoffReplay, TestDistFailureRecovery). Co-Authored-By: Oz <oz-agent@warp.dev>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens distributed-cache test behavior so cluster precondition failures are treated as hard failures instead of benign skips. It focuses on making replication/quorum regressions visible in CI rather than silently bypassing affected tests.
Changes:
- Replaces
t.Skipwitht.Fatalfin read-repair and stale-quorum tests when the expected owner count is not present. - Updates the phase-1 HTTP quorum integration test to fail instead of skipping when node C does not appear to receive the replicated value.
- Expands test comments to document why these preconditions are now expected to hold.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/integration/dist_phase1_test.go |
Tightens the phase-1 quorum integration test and updates its rationale/comments. |
tests/hypercache_distmemory_stale_quorum_test.go |
Converts stale-quorum owner-count precondition from skip to fatal failure. |
tests/hypercache_distmemory_remove_readrepair_test.go |
Converts read-repair owner-count precondition from skip to fatal failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+122
to
+126
| // Replication must reach C within deadline; the previous skip-on-miss | ||
| // branches were placeholders for pre-hint-replay behavior. Locally | ||
| // this completes in ~500 ms across 20 runs. | ||
| if !awaitNodeReplication(ctx, nodeC, "k1", 3*time.Second) { | ||
| it, present := nodeC.Get(ctx, "k1") |
|
|
||
| if !valueOK(it.Value) { | ||
| t.Skipf("value mismatch after wait") | ||
| t.Fatalf("nodeC has wrong value after wait: %T %v", it.Value, it.Value) |
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.
Precondition failures in distributed cache tests (read-repair, stale quorum, and phase-1 basic quorum) were silently swallowed via t.Skip, masking genuine test-environment regressions as benign skips.