-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update workflow with macOS 11 #104
Conversation
Update workflow with macOS 11
Update macOS 11
Update build OS simulator versions
Update device size for ios 13 and create one for ios 14.
Update snapshots for iOS 14 and iOS 13.
Update Xcode version for simulators and CI
Update snapshots
Update snapshots
Update Objc Snapshots
Update Snapshot pod version + update snapshots.
We are having some trouble with snapshots that are apparently the same, but still tests are failing.
* master: Add support for using iOSSnapshotTestCase as the snapshot engine when installing via SPM (#97)
Change Pixel tolerance on FB Snapshots
Replace snapshots for the rendering methods
Update snapshots for iOS 14.5
Update tests sensitivity
Update iOS 14 snapshots
update ios 13 snapshots
Fix per pixel tolerance in simple snapshot tests
This patch is to update the os version our ci will run. A few other files needed to be update, things such as devices sizes, some frameworks versions where also updated. |
Hi, any news regarding this? |
Example/Podfile
Outdated
# SnapshotTesting dropped support for building with Xcode 10 in 1.8.0, so pin the version to 1.7 in order to | ||
# run our tests against Xcode 10. | ||
pod 'SnapshotTesting', '= 1.7.2' | ||
pod 'SnapshotTesting', '= 1.9.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this ~> 1.9
now, since we don't need to pin to a specific version?
@@ -44,8 +44,9 @@ class SnapshotTestCase: FBSnapshotTestCase { | |||
// MARK: - Private Static Properties | |||
|
|||
private static let testedDevices = [ | |||
TestDeviceConfig(systemVersion: "14.2", screenSize: CGSize(width: 390, height: 844), screenScale: 3), | |||
TestDeviceConfig(systemVersion: "13.3", screenSize: CGSize(width: 375, height: 812), screenScale: 3), | |||
TestDeviceConfig(systemVersion: "15.2", screenSize: CGSize(width: 390, height: 844), screenScale: 3), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran the tests locally and it looks like you're missing a bunch of reference images for 15.2. If we're going to add 15.2 support here, we need to have a matching CI build to verify it. This PR is getting quite large, though, so I think it's worth splitting out adding 15.2 into a separate change.
This float ranges from 0 to 1 and will be used to calculate how precise a snapshot test will be, | ||
where 1 means all pixels should match, and 0 means that none of them need to match. | ||
*/ | ||
static public let testPrecision: CGFloat = 0.9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
90% seems pretty low... does it need to be this low to pass, or can we be stricter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good catch. It was a type, but I haven't tested with other numbers. Let's try 98% to see how it goes.
@@ -14,47 +14,47 @@ | |||
// limitations under the License. | |||
// | |||
|
|||
#define SnapshotVerifyAccessibility(view__, identifier__)\ | |||
#define SnapshotVerifyAccessibility(view__, identifier__, perPixelTolerance__)\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep the basic two argument macro and add a new one for tolerance. I think many consumers will still want to non-tolerance version.
Some changes were made by Nick Entin's suggestion, such as updating the snapshot testing version, update macros for objc testing and removing some ios 15 references.
This is missing a bunch of file deletions... we've added reference images for 14.5 and 13.7, but haven't deleted the old ones for 14.2 and 13.3. It also still contains a bunch of reference images for iOS 15 that we shouldn't add yet. I spent way too long trying to figure out why Git wasn't able to figure out a lot of these were renames before I realized that was the issue. 😂 In any case, ended up with #107. |
@@ -46,18 +46,18 @@ enum TaskError: Error { | |||
} | |||
|
|||
enum Platform: String, CustomStringConvertible { | |||
case iOS_15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add 15 yet, since we aren't actually running it here. Better to have one update that adds iOS 15 coverage than slowly add it over multiple PRs and lose track of what's supported and what's not.
At a higher level, this PR is getting quite large. From what I can tell, there's at least X distinct tasks here:
Unfortunately the runners don't overlap nicely, so 2 and 4 are tied together, but I think it would be a lot cleaner to move 1 and 3 (and maybe 5) to separate PRs. Especially 3 since it's a bit of a complicated topic (see #63). |
Update workflow with macOS 11