Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a standardized snapshot testing approach for iOS UI components by adding device-specific configurations. The PR transitions InfoCardSnapshotTests from using .sizeThatFits layout to testing against fixed device configurations (iPhone SE 3, iPhone 15 Pro, and iPad 10th generation).
Changes:
- Added SnapshotHelper.swift with device configuration enum and multi-device assertion helper
- Refactored InfoCardSnapshotTests to use the new assertOnAllDevices helper function
- Generated new device-specific snapshot images for all InfoCard test cases
Reviewed changes
Copilot reviewed 2 out of 26 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Tests/SebGreenComponentsSnapshotTests/SnapshotHelper.swift | New helper file defining device configurations and multi-device snapshot assertion function |
| Tests/SebGreenComponentsSnapshotTests/InfoCardSnapshotTests.swift | Refactored to use assertOnAllDevices instead of individual assertSnapshot calls with .sizeThatFits |
| Tests/SebGreenComponentsSnapshotTests/Snapshots/InfoCardSnapshotTests/*.png | New device-specific snapshot images replacing old .sizeThatFits snapshots |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| enum SnapshotDevice: String, CaseIterable { | ||
| case iPhoneSE3 = "iPhoneSE3" | ||
| case iPhone15Pro = "iPhone15Pro" | ||
| case iPad10th = "iPad10th" | ||
|
|
||
| var config: ViewImageConfig { | ||
| switch self { | ||
| case .iPhoneSE3: | ||
| return .iPhoneSe | ||
| case .iPhone15Pro: | ||
| return .iPhone13Pro |
There was a problem hiding this comment.
The device configuration for iPhone15Pro is mapped to .iPhone13Pro, which may not accurately represent the actual iPhone 15 Pro device dimensions and characteristics. Consider using a more appropriate configuration or creating a custom ViewImageConfig that matches iPhone 15 Pro specifications.
| enum SnapshotDevice: String, CaseIterable { | |
| case iPhoneSE3 = "iPhoneSE3" | |
| case iPhone15Pro = "iPhone15Pro" | |
| case iPad10th = "iPad10th" | |
| var config: ViewImageConfig { | |
| switch self { | |
| case .iPhoneSE3: | |
| return .iPhoneSe | |
| case .iPhone15Pro: | |
| return .iPhone13Pro | |
| import UIKit | |
| enum SnapshotDevice: String, CaseIterable { | |
| case iPhoneSE3 = "iPhoneSE3" | |
| case iPhone15Pro = "iPhone15Pro" | |
| case iPad10th = "iPad10th" | |
| // Custom configuration approximating iPhone 15 Pro dimensions and safe area. | |
| private static let iPhone15ProConfig = ViewImageConfig( | |
| safeArea: UIEdgeInsets(top: 59, left: 0, bottom: 34, right: 0), | |
| size: CGSize(width: 393, height: 852), | |
| traits: UITraitCollection() | |
| ) | |
| var config: ViewImageConfig { | |
| switch self { | |
| case .iPhoneSE3: | |
| return .iPhoneSe | |
| case .iPhone15Pro: | |
| return SnapshotDevice.iPhone15ProConfig |
| // SnapshotHelper.swift | ||
| // SebGreenComponents | ||
| // | ||
| // |
There was a problem hiding this comment.
The file header comment is incomplete. It has an empty "Created by" section on line 5. Either add the author information or remove this line to maintain consistency with the updated InfoCardSnapshotTests.swift file which has this line removed.
| // |
| return .iPhoneSe | ||
| case .iPhone15Pro: | ||
| return .iPhone13Pro | ||
| case .iPad10th: |
There was a problem hiding this comment.
The iPad10th device is mapped to .iPadPro11 configuration. While this may be acceptable if the screen dimensions are similar, consider documenting why this mapping was chosen or if a more accurate configuration should be used for the 10th generation iPad.
| case .iPad10th: | |
| case .iPad10th: | |
| // Use the 11" iPad Pro snapshot configuration for the 10th generation iPad, | |
| // as SnapshotTesting does not provide a dedicated config and the screen size | |
| // is sufficiently similar for our snapshot purposes. |
There was a problem hiding this comment.
I think Copilot has a point here.
|
@Sujataseb I think while this solves the problem of capturing on different screen size, the problem still remains if we use iOS 26.1 simulators vs iOS 18.x simulators as some components still change appearance. And this will cause a problem in team and also CI. If we do not enforce devs to capture screenshot on commonly agreed upon destination. Also I feel it is not necessary to capture components on the actual screen-sized canvas. Is there? Let's talk tomorrow morning or whenever you have time. |
|
My thoughts: In this project, we can see the snapshots will fail if we capture the snapshot for iOS 18.x simulator as a destination and then test it for iOS 26.x simulator. As most components have a different look, for iOS versions. To solve this dev-ex problem and CI setup mismatch with dev setup. We want the tests to still be able to record for the developer on any device they want. But the the test should always fail if the device is not pointing to iPhone 17 Pro (26.1) simulator, with a message that explains that though they are able to record and see how it looks on this device, the recording and testing is supposed to be on iPhone 17 Pro (26.1), as that is what the CI will run the tests on. This will solve the mismatch issues in team and CI and also enable developers to still view their test results on different simulator during development, ensuring the end recordings is always done for CI and team agreed destination. |
No description provided.