Skip to content
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

Closed
wants to merge 25 commits into from

Conversation

alcivanio
Copy link
Collaborator

Update workflow with macOS 11

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)
Update OS version and Devices + snapshots
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
@alcivanio
Copy link
Collaborator Author

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.

@gonzaloperretti
Copy link

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'
Copy link
Collaborator

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),
Copy link
Collaborator

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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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__)\
Copy link
Collaborator

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.
@NickEntin
Copy link
Collaborator

NickEntin commented Oct 6, 2022

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
Copy link
Collaborator

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.

@NickEntin
Copy link
Collaborator

At a higher level, this PR is getting quite large. From what I can tell, there's at least X distinct tasks here:

  1. Update iOS 13 snapshots from iOS 13.3 to 13.7.
  2. Update iOS 14 snapshots from iOS 14.2 to 14.5.
  3. Introduce comparison tolerances.
  4. Update the CI runner from macOS-10.15 to macOS-11 and associated gem/action dependencies.
  5. Change sample project dependencies to use SnapshotTesting 1.9 instead of 1.7.2.

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).

@NickEntin NickEntin closed this Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants