Repository navigation
test(apiv2): cover the v2 paths the migration left untested - #401
Conversation
Several tests still exercised v1-era code after the move to /api/v2, so the live v2 paths had no coverage: - Image size: tests used only v1 `schema_version` payloads; the v2 `state` branch was untested. Drop the unreachable v1 branch and test the vendored v2 fixture. - Item videos: tests decoded a path production no longer uses. Test `ItemDetail(catalog:)` on the vendored v2 item fixture instead, and drop the always-nil legacy `FileVersion.edition` fallback. - Playback start: tests encoded `PlaybackV3StartRequest`, not the body production sends. Check the sent `APIv2PlaybackStartBody` members against the server's OpenAPI schema instead. - Auth, pairing and push tests pasted server fixtures by hand, and one had drifted. Vendor them through sync-apiv2-fixtures.sh at the same server commit. - Pairing: pin literal wire frames so a key renamed on both sides of the codec fails. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (28)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Quick104 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
Tests now cover the v2 code paths the
/api/v2migration left untested, and two leftover v1 fallbacks are gone.Problem
An audit of the test suite after the v2 migration (#349–#398) found tests that still exercised v1-era code while the live v2 path had no test:
ImageSizeCapabilityTestsandArtworkURLTestsused only v1-shaped payloads (schema_version: 1, nostate). The v2 branch inImageSizeSelection.queryEntries(state == "available") had no test, so breaking it would have sentimage_size=largeto servers that don't offer it. v2 never sendsschema_version, so no server could reach the v1 branch.ItemVideosDecodingTestsdecodedItemDetailthrough its hand-writteninit(from:), which no production read calls.SiloAPI.itemDetailbuildsItemDetailthroughItemDetail(catalog:), andItemVideo(catalog:)had no test.DetailVersionSelectionTeststested a legacyFileVersion.editionfallback, but the v2 projection always setseditionto nil.PlaybackProtocolV3TestsencodedPlaybackV3StartRequestwith a test-owned encoder. Production sendsAPIv2PlaybackStartBody, so a missing or extra member in the real body would pass.AuthDeviceV2Tests,PairingDeviceAPITestsandApplePushRegistrationTestscarried hand-pasted copies of server fixtures that the sync script doesn't update. The device-login capability copy had already drifted:"revision": "1", where the server sends a hash. The tests also patched these strings withreplacingOccurrences, which does nothing without warning if the server reformats a fixture.PairingProtocolTestsonly round-tripped messages through one codec, so a key renamed on both sides would pass while breaking older phones and TVs and the Android client.Solution
ImageSizeCapabilityResponsedropsschemaVersion, andstateis now required.queryEntriesaccepts onlystate == "available". The capability is held only in memory, so no stored copy can lackstate. Tests decode the vendoredget_image_capabilities_okfixture with the production decoder. They cover every serverstate(disabled,not_configured,unsupported, and an unknown value) and assert that a v1 document is rejected.ArtworkURLTestsnow expects that rejection.get_catalog_item_okfixture, addvideosandextras, and run it throughCatalogItemDetaildecoding andItemDetail(catalog:), the same pathSiloAPI.itemDetailuses. A new case checks that a video without the schema-requiredis_officialfails the read. The trailer-refresh "queued" case reads the vendoredrefresh_catalog_item_trailers_okfixture.FileVersion.editionand its fallback ineditionDisplayLabel. The edition tests now useedition_rawandedition_key.OverlaySummary.editionis a different field and is unchanged.PlaybackSessionTransportTestsnow checks the bodystartPlaybackactually sends againstPlaybackStartBodyandClientPlaybackContextV3from the server's OpenAPI document at the pinned commit. Every required member must be present and no member outside the schema may appear, because both schemas setadditionalProperties: false. It also checks that an explicit zerostart_positionis sent.SELECTEDinscripts/sync-apiv2-fixtures.shand re-synced at the same server commit,84ed9e596. The re-sync only added files and left the existing fixtures byte-identical. The 3 test files load these instead of pasted copies. The newAPIv2FixtureTestSupport.text(named:bundleClass:setting:)overrides top-level members on the parsed JSON rather than editing raw text.PairingMessageCodec.APIv2ContractTestsfrom 34 to 43./api/v1sweep: the only v1 request left is the unauthenticatedGET /api/v1/healthprobe, which the 2026-09-07 decision recorded in silo-server'scontracts/api/v2/migration.jsonkeeps.scripts/ci/check-no-api-v1.shpasses unchanged.Validation
SiloTests, iPhone 17 Pro, iOS 27.0 simulator): 1979 tests, 0 failures, 2 skipped.SiloTVTests, Apple TV 4K (3rd generation), tvOS 27.0 simulator): 1790 tests, 0 failures, 2 skipped. TheSiloTVbuild includes the Top Shelf extension, which sharesImageSizeCapabilityResponse.-parallel-testing-enabled NO, and a 60 s per-test allowance.isOfficialto false, renaming the start body'sinstallation_idkey, and accepting any non-empty imagestatetogether caused 8 test failures acrossItemVideosDecodingTests,PlaybackSessionTransportTestsandImageSizeCapabilityTests.bash scripts/ci/check-no-api-v1.test.sh,bash scripts/ci/check-no-api-v1.shandbash scripts/sync-apiv2-fixtures.test.shpass.Validation tasks: none
Risks and follow-ups
/api/v2/images/capabilitieswithoutstateno longer getsimage_size=large. Every v2 server sendsstate.PlaybackSessionTransportTestscopies the start-body member lists from the OpenAPI document at the pinned commit. When the fixtures are re-synced at a newer server commit, these lists need a manual update.Decodableinitializers forItemDetailandFileVersioninModels.swiftlook unused now that reads go through the v2 projection. Removing them is a separate refactor and isn't part of this PR.Code and description written by Claude Opus 5.5 (
claude-opus-5-5) in Claude Code (T3 Code). Claude Opus 5.5 subagents audited the test suite to find these gaps.🤖 Generated with Claude Code