PP-4542: 3.2.0 regression test-quality fixes (F-003/F-005/F-006/F-007/F-008)#1052
Open
mauricecarrier7 wants to merge 7 commits into
Open
PP-4542: 3.2.0 regression test-quality fixes (F-003/F-005/F-006/F-007/F-008)#1052mauricecarrier7 wants to merge 7 commits into
mauricecarrier7 wants to merge 7 commits into
Conversation
…ve XCTSkipIf dodge (F-003 / PP-4542)
The PP-4407 awaitReady() migration moved BookRegistrySync.sync()'s loansUrl
read out of the synchronous guard and into the post-awaitReady() Task. The
old `guard let loansUrl = currentAccount.loansUrl else { return }` was a clean
synchronous no-op; the new code reaches setState(.loaded) + completion(nil,false)
for the "current account present but no loansUrl" case — either synchronously
(no stored credentials) or after awaitReady() resolves to AccountDetails with a
nil loansUrl (anonymous library).
Two tests were band-aided with `try XCTSkipIf(currentAccount?.loansUrl != nil)`,
so they false-greened on a signed-in CI simulator and only ran (and would only
catch the regression) in a clean environment. That is illusory coverage on the
registry-sync critical path (TPPBookRegistry is the single source of truth for
checkouts/holds/reading position).
This replaces the dodge with deterministic, host-independent behavior tests
that inject a fresh-UUID fixture account (so the production-stack credentials
lookup — TPPUserAccount.sharedAccount(libraryUUID:) → production manager's
userAccount(for:) — returns a never-signed-in instance regardless of host
sign-in state):
- test_sync_withCurrentAccountButNoCredentials_resolvesToLoadedSynchronously
Asserts the NEW synchronous credentials-gate contract: setState exactly
[.loaded], completion(nil,false), syncUrl stays nil. No keychain needed.
- test_sync_whenAlreadySyncing_returnsBeforeSettingSyncingState
Drives the re-entrancy guard, which is only REACHABLE once credentials exist
(it sits after the credentials gate). Writes credentials to the exact
production-manager instance sync() reads, drives awaitReady to a terminal
detailsLoaded(nil loansUrl), and asserts currentState == .syncing
short-circuits before setState. Keychain-gated (environment-capability guard,
not a host-state dodge).
- test_sync_whenNotSyncing_withCredentialsAndNoLoansUrl_resolvesToLoaded
Contrast: SAME credentialed/no-loansUrl fixture but currentState == .loaded
proceeds into the Task and resolves to .loaded — proving the .syncing guard
(not the credentials) is what suppresses setState.
Each assertion would fail under a real regression of the guard logic. No
production code changed — the new transient setState behavior is not
user-harmful (loaded → loaded for an account that was already loaded), so
option (a) honest-test was correct over option (b) production change.
**Scope:** test-only. PalaceTests/Book/BookRegistrySyncTests.swift.
Verified: -only-testing:PalaceTests/BookRegistrySyncTests → ** TEST SUCCEEDED **,
all 3 new tests pass (not skipped). check-test-name-vs-body.py exit 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n mutant coverage PR #1028 added the stale-position-on-reborrow hardening in AudiobookSessionManager (validationFailure / fallback-to-most-recent bookmark). The changed lines shipped with a 0% diff-only mutation kill rate — the position-validation and candidate-selection logic on the audiobook-playback critical path was unasserted. New behavior tests at the production seams pin the real decisions with constructed TOC + registry + account fixtures (no live Audiobook/player graph, so the toolkit's fragility doesn't leak into the suite): - candidate recency ordering: a reverse sort ('<') now returns the OLDEST bookmark and fails testSelectMostRecentValidBookmark_* - TOC track-key match: in-manifest vs foreign key asserted both ways - validationFailure==nil filter inside selectMostRecentValidBookmark: a cap-exceeding (valid-key, reconstructs) candidate must be dropped and the valid older one selected — kills the filter-inversion mutant - isValidPosition valid/invalid both sides - isUserAuthenticated auth-doc-load-failure -> not-authenticated To make the candidate filter+sort testable without a live Audiobook, extracted the pure selection step into selectMostRecentValidBookmark (from: [TPPBookLocation], in: AudiobookTableOfContents); the four seam methods are now `internal` (not `private`) so @testable tests reach them — mirrors the existing static-policy-mirror pattern in this file. Mutation (diff-only vs origin/release/3.1.0, full changed-line coverage across AudiobookPositionRestoreTests + the existing AudiobookNetworkValidationTests + AudiobookLoadFailureSAMLReauthTests): 0% -> 88.9% (8/9 killed). The lone survivor is line 1324 '>' -> '>=' on candidates.sorted: with distinct ISO8601 timestamps '>' and '>=' produce identical output, and on a tie either bookmark is equally "most recent" — killing it would require asserting an arbitrary Swift sort-stability detail with no behavioral contract, which is banned fluff. Documented as an equivalent mutant, not covered. **Scope:** Test-coverage fix on a critical path (audiobook position restore). Production change is visibility-only + one pure-function extraction (no behavior change): 4 methods private->internal, plus fallbackToMostRecentValidBookmark delegating to the extracted selectMostRecentValidBookmark. No call-site, signature, or control-flow change to shipping behavior. **Not done:** the 1324 '>'->'>=' tie-break mutant (equivalent — see above). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds discriminating, behavior-asserting tests for two 3.2.0-regression
test-quality findings surfaced by diff-only mutation testing. Test-only
change — no production code touched.
F-006 DownloadAuthRetryHandler (foreign-host 401 guard, lines 234/240):
- testHandle_401_foreignHost_browserSAML_shortCircuitsReturnsFalse_noReauth
- testHandle_401_accountHost_browserSAML_guardDoesNotFire_drivesSAMLRetry
- testHandle_403_foreignHost_noCredentials_guardDoesNotIntercept_signInStillFires
Kills :234 ==->!= and :240 return false->true. diff-only kill rate 100% (3/3).
F-008 SAML/browser-auth branch detection:
- BookReturnService :476 isBrowserBased==true (markCredentialsStale gate):
testReturnBook_SAMLBrowserAuth_invalidCredentials_marksCredentialsStaleBeforeReauth
+ basicAuth negative control. diff-only 100% (2/2), :476 KILLED.
- TokenRefreshInterceptor :166 + :216 isSaml==true (SAML-vs-browser legacy
dispatch): two tests asserting .SAMLStarted+startDownload+no-reauth vs
.downloadNeeded+reauth. Both :166 and :216 KILLED.
- BorrowOperation :636 needsBrowserReauth (isBrowserBased==true && hasCreds):
testBorrow_SAMLBrowserAuth_withCredentials_routesToReauthModalNotAlert.
:636 ==->!= and &&->|| both KILLED (file 33%->63.6%).
**Scope:** test-only; 4 PalaceTests files; +417 LOC.
**Not done (deliberate, anti-fluff per CLAUDE.md):** BorrowOperation :685/:677
(isSaml==true selecting authLabel/reason) are cosmetic — authLabel is
log-only and reason collapses to the same AuthCoordinator.routeName "modal"
route for a SAML mechanism, so no observable downstream difference. :581/:592
are logically dead: the isAuthError closure unconditionally returns true at
:592, so :581 flips change nothing observable. Killing these would require
fluff or a production-logic change out of F-008 scope.
**Deferred:** none.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…diobook-position-coverage' into fix/pp4542-bookregistrysync-contract
…4542)
XCTestConfigurationFilePath / environment["XCTest..."] / NSClassFromString("XCTest")
guards are unkillable by design (suite always runs under XCTest), so counting
them as mutants deflates kill rate with no real coverage signal. Mirrors the
existing _LOG_LINE_PATTERN skip. TPPReauthenticator.swift:66 was the canonical
false-0%: now correctly reports 0 mutable points on the changed line.
**Scope:** tooling only (scripts/palace_mutate.py); no production or test code.
Verified: diff-only enumeration on TPPReauthenticator drops 1->0 mutants.
…hangeset ForgeOS changeset: cs_7e7ab728 Reviewed-by (architect): rev_39f29990 APPROVED (extraction behavior-preserving; internal-not-public visibility; no behavior change) Reviewed-by (qa_test): rev_7114cb1a APPROVED (XCTSkipIf dodge removed; mutation kills credible; deliberate skips legitimate) Gates passed: review, testing, release (proj_87884c17). Critical-path file touched: Palace/Audiobooks/AudiobookSessionManager.swift. **Scope:** governance record only (empty commit). Non-blocking follow-ups: F-009 (BookReturnService:235 task! force-unwrap, pre-existing, separate changeset); QA warning on fixed-poll async test loops.
🏗️ CodeAtlas Ledger Analysis✅ All Checks Passed♿ Accessibility (via AccessLint)
✅ No accessibility issues detected 🧪 Test Coverage (via QAAtlas)
🏛️ Architecture Analysis
🔍 Reachability Analysis
✅ No dead code detected 📊 0 files analyzed | 📦 Download Full Report Powered by CodeAtlas Ledger |
🧪 Unit Test Results📊 View Full Interactive Report ❌ TESTS FAILED📊 Testing Coverage Breakdown
🔗 Interactive HTML Report | CI Run Details 📦 Downloadable Artifacts
|
…007/F-003 tests CI build-and-test on #1052 failed the static isolation lints (not a flake): - AudiobookPositionRestoreTests used a bare `AccountsManager()` (via a `makeIsolatedAccountsManager()` helper) — the exact polluter the lint guards (bare init spins a background loadCatalogs Task that outlives the test). Fixed: build the manager through the whitelisted `makeTestAppContainer` (pins deferInitialLoadCatalogsForTesting, so no background work) and rename the helper to `makeIsolatedManager()` so its call sites don't trip the substring `AccountsManager(` matcher. - BookRegistrySyncTests' 3 `AppContainer.production().accountsManager .userAccount(for:)` credential writes are legitimate: sync() checks hasCredentials() via sharedAccount→production keychain-backed userAccount (only currentAccount STATE is injected). Marked `// MIGRATED-DEFERRED:` per the lint's documented escape for "production resolution IS the test contract". Verified: AccountsManagerIsolationLintTests + AppContainerIsolationLintTests + AudiobookPositionRestoreTests + BookRegistrySyncTests = 42 tests, 0 failures. **Scope:** test-only; no production change. Resolves the #1052 CI red.
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
1 participant
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.
Test-quality fixes surfaced by the 3.2.0 pre-release regression (PP-4542) against the
release/3.2.0candidate. Predominantly test additions that kill specific surviving mutants; one behavior-preserving extraction enables the audiobook coverage.Findings addressed
awaitReady()migration changedsync()so the no-loansUrlcase transitionssetState(.syncing) → .loadedinstead of being a synchronous no-op. Two baseline tests were band-aided withXCTSkipIf(currentAccount?.loansUrl != nil)— so they skipped on a signed-in CI sim (false green) and failed in a clean env. Replaced the skip-dodge with deterministic fixture-injected tests that assert the real transient-state contract.selectMostRecentValidBookmark(from:in:)+ 5private→internalseam exposures (no behavior change).authDef?.isSaml/isBrowserBased == truesurvivors across BookReturnService / BorrowOperation / TokenRefreshInterceptor with SAML-vs-OAuth discriminating test pairs. Cosmetic/dead/equivalent mutants were deliberately NOT chased (documented).XCTestConfigurationFilePath, etc.) are unkillable by design; now skipped like log lines, so they stop deflating kill rate (TPPReauthenticator was a false 0%).Verification
origin/release/3.1.0: DownloadAuthRetryHandler 33%→100%, BookReturnService→100%, AudiobookSessionManager 0%→88.9%, TokenRefreshInterceptor/BorrowOperation SAML branches killed.cs_7e7ab728: architect (rev_39f29990) + qa_test (rev_7114cb1a) approved; review/testing/release gates passed.Notes / follow-ups (not in this PR)
BookReturnService.swift:235force-unwrapstask!before theTask{}assignment — an IUO-nil race in the return path. Surfaced while writing the return-path tests; needs a separate production fix.Jira: PP-4542