-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Added ScreenshotTestHelpers #2682
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
Conversation
|
Can you add a simple test for |
ifosli
left a comment
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.
Overall, nice start! I left some comments; to summarize, my main concerns are:
- Is it possible to simplify the test helper so the caller can do less work?
- We'll need to also add support for swiftUI tests
- Let's make the snapshot names shorter, please!
Feel free to push back against anything I've suggested that you disagree with or that seems too hard/annoying, though!
Kickstarter-iOS/Features/LoginTout/Controller/LoginToutViewControllerTests.swift
Show resolved
Hide resolved
...ToutViewControllerTests_testLoginToutView_pad_ja_light_UICTContentSizeCategoryM_portrait.png
Show resolved
Hide resolved
ifosli
left a comment
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.
The overall strategy looks good now, but I still think the names are too long to be practical. I think it might be useful to have a team discussion around what info we want in test names and why - I know Amy well enough to know that she's got good reasons for wanting all the fields in the name, but I don't know what they are personally.
| lang=\(type.language.rawValue), | ||
| style=\(type.style.snapshotDescription), | ||
| font=\(type.contentSizeCategory.rawValue), | ||
| orientation=\(type.orientation.snapshotDescription) |
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.
Nice!
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.
This is great, thanks for adding!
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 still find these file names to be way too long to be practical, at least in my personal work flow. Maybe it's worth checking with the team what info they find necessary in a snapshot test? Personally, I don't think things like device type and size category are useful enough to be worth the extra words, but I'm okay with being outvoted.
Even without removing information, there's still some things you could do to make the name more streamlined. The full path of this one is ios-oss/Kickstarter-iOS/Features/LoginTout/Controller/__Snapshots__/testLoginToutView.LoginToutViewControllerTests_testLoginToutView_phone4-7inch_en_dark_UICTContentSizeCategoryAccessibilityXXXL_portrait.png. Notice how this includes the test name twice? I also think it'd be worth describing the content size category more concisely if we're going to include it at all - maybe as accessibilityXXL?
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 can drop the file/test prefix from the named: value so the filename is just the config (device/lang/style/font/orientation), and let the test’s Snapshots/testLoginToutView/ folder provide the test context. I’ll also shorten the content-size labels—thinking a11yXXXL as a common shorthand for “accessibility” to keep names shorter, but I can spell out “accessibilityXXXL” if you prefer. With these tweaks, a filename would look like phone4_7inch_en_dark_a11yXXXL_portrait.png. If the team wants to trim further (e.g., drop style/font), I can do that too.
Generated by 🚫 Danger |
ifosli
left a comment
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.
Nice job on this! I have just one final request to shorten the device name, too, but otherwise I think everything is good to go! Thanks for working so hard on this! If you want to merge this pr, please just make sure you delete the extra screenshots first (the ones with old names).
📲 What
🤔 Why
🛠 How
-UIViewController
-UIView (optional intrinsic sizing)
-SwiftUI View (optional intrinsic sizing)