Skip to content

test: cover FallbackChainService behavior - #43

Open
harshitethic wants to merge 8 commits into
macOS26:mainfrom
harshitethic:test/fallback-chain-service
Open

test: cover FallbackChainService behavior#43
harshitethic wants to merge 8 commits into
macOS26:mainfrom
harshitethic:test/fallback-chain-service

Conversation

@harshitethic

Copy link
Copy Markdown

Summary

  • add coverage for FallbackChainService failure thresholds
  • cover fallback progression and disabled entries
  • cover success, reset, and clear state transitions
  • cover summary and FallbackEntry display behavior
  • isolate persisted UserDefaults state between tests

Closes #28

@macOS26 macOS26 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran the suite locally on macOS: xcodebuild test -only-testing:AgentTests/FallbackChainServiceTests → 10/10 passed (✔ Test run with 10 tests in 1 suite passed).

I verified every assertion against Agent/Services/FallbackChainService.swift and they're all correct reads of the state machine: maxFailuresBeforeFallback = 2 (first failure → nil, second → enabledChain[0] with consecutiveFailures reset), the enabled-only filtering in recordFailure/activeFallback/summary, recordSuccess/reset returning to primary, clear wiping chain + state, and the "→ " active marker in summary. The FallbackEntry.displayName raw-provider fallback test is right too. The test hygiene is genuinely good: @Suite(.serialized), and the withIsolatedService snapshot/restore of both the singleton state and the raw UserDefaults keys is careful (the raw-data restore at the end makes the re-added-entries-get-new-UUIDs detail harmless).

One requested change — out-of-scope deletions. The diff removes ~25 lines of explanatory comments from existing, unrelated tests: the allCriteriaDone gate rationale in GoalStateStoreTests, the entire doc comment on ToolFailureDetectionTests explaining the twice-broken isToolFailure history (including why the bare word "failed" was deliberately dropped as a trigger), and the polling-exemption rationale in StuckGuardFingerprintTests. Those comments document regression history — they're the reason future contributors won't reintroduce those exact bugs. Please revert every change above the // MARK: - FallbackChainService line so the diff is purely additive (the one whitespace fix at line 111 is fine either way).

Also: the PR body says Closes #28, and PR #40 claims the same issue. Since FallbackChainService genuinely had zero coverage, this PR is the better fit for #28 — I've asked #40 to drop its claim, so keep yours.

Everything else is merge-ready; happy to approve as soon as the comment deletions are reverted.

@macOS26 macOS26 closed this Aug 30, 2026
@macOS26 macOS26 reopened this Aug 30, 2026
@macOS26 macOS26 closed this Aug 30, 2026
@macOS26 macOS26 reopened this Aug 30, 2026
@macOS26 macOS26 closed this Aug 30, 2026
@macOS26 macOS26 reopened this Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit tests for an untested service in Agent/Services/

2 participants