test: stop the artwork tests using the user's real cache directory (#364) - #368
Merged
Conversation
cachedImageIfAvailable recomputed the default cache path inline, so the only way to exercise it was to write into ~/Library/Caches/Spotiglass/Artwork, the same directory the app and the rest of the suite share. Anything that trimmed or cleared that cache mid-run made the test fail, and running the suite edited the caches of whoever ran it. The location is now defined once and the read takes the directory as a parameter.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are small but concrete maintainability issues (duplicate cache-root resolution in ArtworkImageStore init and a brittle path-suffix assertion in tests) with straightforward fixes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens artwork-related tests by eliminating reliance on the user’s real cache directory and centralizing the production disk-cache location so both async and synchronous read paths agree.
Changes:
- Centralizes the artwork disk-cache directory as
ArtworkImageStore.defaultDiskDirectoryand uses it as the default forcachedImageIfAvailable(for:diskDirectory:). - Updates artwork cache tests to use per-test temporary directories instead of
~/Library/Caches/.... - Updates the immersive blurred artwork view test to inject a pre-decoded
initialImagerather than depending on cache timing.
File summaries
| File | Description |
|---|---|
| SpotiglassTests/ImmersiveLyricsViewsTests.swift | Moves the blurred-artwork test off the user cache and injects initialImage for deterministic hosting/inspection. |
| SpotiglassTests/ArtworkImageStoreTests.swift | Verifies synchronous cache reads against an explicitly provided directory and asserts the default cache location expectation. |
| Spotiglass/Utilities/ArtworkImageStore.swift | Introduces defaultDiskDirectory and adds a diskDirectory default parameter to the synchronous cache read API. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
24
to
30
| private init() { | ||
| let base = FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask).first | ||
| ?? FileManager.default.temporaryDirectory | ||
| let disk = base | ||
| .appendingPathComponent(AppMetadata.displayName, isDirectory: true) | ||
| .appendingPathComponent("Artwork", isDirectory: true) | ||
| let disk = Self.defaultDiskDirectory | ||
| let urlCacheDir = base.appendingPathComponent(AppMetadata.displayName, isDirectory: true) | ||
| .appendingPathComponent("ArtworkURLCache", isDirectory: true) | ||
| try? FileManager.default.createDirectory(at: urlCacheDir, withIntermediateDirectories: true) |
Comment on lines
+178
to
+181
| XCTAssertTrue( | ||
| ArtworkImageStore.defaultDiskDirectory.path.hasSuffix("/\(AppMetadata.displayName)/Artwork"), | ||
| "The shared store and the synchronous read must resolve the same cache directory" | ||
| ) |
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.
Part of #364, follows #366.
ArtworkImageStore.cachedImageIfAvailable(for:)recomputed the default cache path inline, duplicating the shared store's own initializer. Tests could therefore only exercise it by writing into~/Library/Caches/Spotiglass/Artwork, the directory the app and the rest of the suite share: anything that trimmed or cleared it mid-run madetestBlurredArtworkHostsAndLoadsFromDiskCachefail, which is the intermittent failure recorded in #364. It also meant running the suite edited the caches of whoever ran it.ArtworkImageStore.defaultDiskDirectory, used by both the shared store and the synchronous read.cachedImageIfAvailable(for:diskDirectory:)takes the directory, defaulting to that shared location, so production call sites are unchanged.ImmersiveBlurredArtwork(url:initialImage:)instead of relying on cache timing.Not run locally: only
build-for-testing, which succeeded. CI is the verdict.Remaining in #364: a per-test watchdog so a hang fails fast instead of stalling the job.