Conversation
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/689")Built from c80ff5b |
e6493f1 to
4b1df73
Compare
4b1df73 to
c822f7c
Compare
`TranscodingProcessor` duplicated `ResizingProcessor` — same `.processed(_, mimeType: "video/mp4", filename: "clip.mp4")` result, one call site — and was the weaker of the two. It wrote to a fixed `$TMPDIR/clip.mp4` instead of a per-call UUID path inside the managed upload directory, and swallowed the write with `try?`, so a failed write still returned `.processed(<nonexistent URL>, …)` and the test passed green against a file that never existed. `ResizingProcessor` uses `try` and a unique path. Also drops `@unchecked Sendable` from `ThrowingUploader`, which has no stored properties and so satisfies `MediaUploader`'s inherited `Sendable` conformance on its own. The escape hatch is only needed by the mocks holding `NSLock`-guarded state; carrying it on a stateless one normalizes it as boilerplate, which is how an unsynchronized property gets added later without a diagnostic. `ContentTypeDeleteClient` keeps it — it subclasses `InternalMediaClient`, itself an `@unchecked Sendable` class, and must restate the conformance.
c822f7c to
c80ff5b
Compare
dcalhoun
left a comment
There was a problem hiding this comment.
Looks good. Captured one finding from Claude that is worth considering.
|
|
||
| /// A processor that transcodes, used to check the server holds it across the whole | ||
| /// request rather than re-reading a reference the host may have dropped. | ||
| private final class TranscodingProcessor: MediaProcessor, @unchecked Sendable { |
There was a problem hiding this comment.
Finding from Claude:
Consolidating onto one mock is right, but this deletes the better name. The survivor, ResizingProcessor (:1196), returns .processed(_, mimeType: "video/mp4", filename: "clip.mp4") — a transcode, not a resize — and processesThenDelivers feeds it photo.jpg/image/jpeg. Android's equivalent mock is still TranscodingProcessor (MediaUploadServerTest.kt:1002), so a repo-wide grep for that name now returns Android only.
Worth catching while you're here: 42bdb8d2 inserted ValueTypeProcessor between the transcode doc comment and the class it described, so :1184 now stacks two doc comments on ValueTypeProcessor and leaves ResizingProcessor with none.
Renaming the survivor to TranscodingProcessor and re-attaching that comment would make this a pure consolidation and restore parity with Android.
Stacked on #688.
What?
Deletes the
TranscodingProcessortest mock and points its one call site at the existingResizingProcessor. Drops@unchecked SendablefromThrowingUploader. Test-only: one file.Why?
TranscodingProcessorwas a second copy ofResizingProcessor— both return.processed(_, mimeType: "video/mp4", filename: "clip.mp4"), and each had exactly one call site — and it was the weaker copy on two counts.A failed write still produced a passing test. It wrapped its write in
try?and returned.processed(<URL>, …)either way, so when the write failed the test asserted against a file that was never created, and passed.ResizingProcessorusestry, so the same failure surfaces as a thrown error instead of a green run.It wrote to a fixed path it did not own.
TranscodingProcessorwrote to$TMPDIR/clip.mp4— constant across runs and shared process-wide. The server deletes whatever URL a processor hands back (that cleanup lives insideprocessAndUploadso the throw paths are covered too), so the mock was pointing the server at a shared filename to unlink.ResizingProcessorwritesprocessed-<UUID>into the same managed uploads directory the server already created for the incoming file, so the file the server deletes is one the processor made for that request.ThrowingUploaderdid not need@unchecked Sendable. It has no stored properties — a nestedstruct Failure: Error {}and one method — andMediaUploaderalready refinesSendable, so the checked conformance holds on its own. The escape hatch is only needed by the mocks carryingNSLock-guarded state (ProcessOnlyProcessor,DeclineByMetadataProcessor,ResizingProcessor,MockInternalMediaClient). Carrying it on a stateless one normalizes it as boilerplate, which is how an unsynchronized property gets added later without a diagnostic.ContentTypeDeleteClientkeeps it — it subclassesInternalMediaClient, itself an@unchecked Sendableclass, and must restate the conformance.How?
ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift:
TranscodingProcessordeleted;processesForHostReleasedProcessor— "still processes for a processor the host has dropped its reference to" — now builds aResizingProcessor.@unchecked Sendabledropped fromThrowingUploader.The swap changes nothing the test admits or asserts.
TranscodingProcessordeclaredhandlesFilereturningtrue;ResizingProcessoromits it and inherits the protocol default inMediaHandlers.swift, which also returnstrue, so the upload still clears the admission gate. Both mocks return the same processed metadata, so all four#expects stand as written — including the one carrying the test's meaning,!mockUploader.passthroughUploadCalled.The test is still load-bearing, confirmed by mutation rather than by assertion. Holding the handler's processor weakly fails all four expectations,
!passthroughUploadCalledamong them. Reaching that mutation took an@unchecked Sendableweak box:MediaProcessoris not class-bound, soweak var processor: (any MediaProcessor)?does not compile, andHandlermust staySendable, so a bareweak var … : AnyObject?does not either. Two type-system barriers now stand between this code and the regression the test was written for; the test is the third.Testing Instructions
No reviewer steps — the diff is test-only and the suite below exercises it.
swift test --filter MediaUploadServerTests— 38 tests in 3 suites, greenswift test— 983 green:GutenbergKitTests587 in 35 suites,GutenbergKitHTTPTests396 in 22 suitesswift build --build-tests— clean, and confirmsThrowingUploadersatisfiesSendablewithout the annotationmake lint-swift— no violationsRelated
Follow-up from a review of #625, which sits upstream in this stack and is still open. Two other findings from that review are fixed there rather than here:
stop()now drops the listener's connection handler, so teardown releases what the handler captured on the caller's thread instead of Network.framework's queue; and the#WeakMutabilitywarning.#625 also deleted
EditorViewControllerMediaLifetimeTests, vacuous for the same class of reason since it never loaded the editor, and with itLifetimeProbeDelegate. This mock was the remainder.