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

#I Improve snapshot test #783

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Conversation

rasmustautsglia
Copy link
Contributor

@rasmustautsglia rasmustautsglia commented Sep 25, 2023

This PR improves the way snapshot tests are executed. By creating an
extension to UIViewController, we can now create snapshot tests with
less code, time, and effort. And, it is much more readable. Since our
usages of snapshot tests are very limited (for now), this approach serves
those purposes very well.

This commit also updates the following:

  1. Naming of files - some voiceover test files did not have a proper name.
  2. Naming image references - voiceover image references didn't have a
    correct naming, leaving out the dimensions of the view. This PR fixes that
  3. AccessibilityImage tests didn't have frame that was aligned with layout
    and extra3LargeFont tests
  4. Fixes the issue, that some landscape images came out portrait before.

Snapshots PR: https://github.com/salemove/ios-widgets-snapshots/pull/50

MOB-2696

@sm-deployer sm-deployer force-pushed the Improve-Snapshot-tests branch 2 times, most recently from 0f5807d to b22b684 Compare September 25, 2023 13:04
Copy link
Contributor

@EgorovEI EgorovEI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me. More convenient

@EgorovEI
Copy link
Contributor

Please add FIRM tag to PR title

@rasmustautsglia rasmustautsglia changed the title Improve snapshot test #I Improve snapshot test Sep 26, 2023
Copy link
Contributor

@igorkravchenko igorkravchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we have discussed earlier, in my opinion, extending UIViewController to get snapshot references, makes all snapshots very specific - meaning that we will be forced to create same extension for UIView, SwiftUI.View etc, over and over. I think we should instead extend Snapshotting or SnapshotTestCase to keep snapshot matching value generic as it is right now in assertSnapshot<Value, Format> and get desired behaviour via method overload.

@rasmustautsglia
Copy link
Contributor Author

Please re-review as the complete update has been added

Copy link
Contributor

@igorkravchenko igorkravchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think assertSnapshot in more appropriate method name than testSnapshot.

@rasmustautsglia
Copy link
Contributor Author

I think assertSnapshot in more appropriate method name than testSnapshot.

fixed d23621c

This PR improves the way snapshot tests are executed. By creating an
extension to UIViewController, we can now create snapshot tests with
less code, time, and effort. And, it is much more readable. Since our
usages of snapshot tests are very limited (for now), this approach serves
those purposes very well.

As this proposal is up for debate, I would like to hear everybody's input
on it. Please share what are your thoughts on making this change.

MOB-2696
@rasmustautsglia rasmustautsglia merged commit 842c332 into development Sep 27, 2023
1 check passed
@rasmustautsglia rasmustautsglia deleted the Improve-Snapshot-tests branch September 27, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants