-
Notifications
You must be signed in to change notification settings - Fork 341
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
Snapshot preview reuse #3484
Snapshot preview reuse #3484
Conversation
Present it from SettingsDetail and Debug screens; Add wrapper to LocationHistoryDetailViewController.
…nHistoryDetail; Add syncing between navigation items; Remove unnecessary Eureka import and TypedRowControllerType conformance.
…screen to avoid moveDelegate from being released too early; Pass AppEnvironment so LocationHistoryDetail can observe changes to the data set.
- use Current directly - move clear() method below init
…ld not have any restrictions.
Rearranged Utilities tests; Add missing super call in ClientEventTests
Use Current.date() when creating a LocationHistory entry.
- update OS to the current base version (18.2) - change assertion in ClientEventTests to address flakyness
…ht and dark mode; Adopt it in LocationHistoryListViewSnapshot and WidgetsSnapshot tests; Improve snapshot naming in WidgetsSnapshot tests.
…esting; Create SharedTesting framework to hold shared code for testing; Adopt new technique in LocationHistoryListView.
Improve naming; Improve snapshots of Form sections; Improve code reusability.
…use' #Conflicts: # Tests/Widgets/WidgetsSnapshot.test.swift
Can you split this into more than 1 PR? One for the sharing new target, one for location history and one for widgets. |
I can. First let me know if this filter helps https://github.com/home-assistant/iOS/pull/3484/files?file-filters%5B%5D=.h&file-filters%5B%5D=.lock&file-filters%5B%5D=.pbxproj&file-filters%5B%5D=.swift&file-filters%5B%5D=.xcscheme&file-filters%5B%5D=No+extension&show-deleted-files=true&show-viewed-files=true It shows you only the non-snapshot changes. Also, it's important to note that snapshots have not changed. You can see from the navigation on the left side of the screen (if on web) that they were either removed or renamed. The ones removed were the ones for unsupported number of tiles. |
It looks fine to me, but I would at least move the new framework-add to a separate PR, so it's easier to revert if needed and also separate purposes for future reference. |
Will do. A PR adding the framework plus logic and another using the new tooling. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3484 +/- ##
==========================================
+ Coverage 45.46% 45.51% +0.04%
==========================================
Files 224 224
Lines 13355 13355
==========================================
+ Hits 6072 6078 +6
+ Misses 7283 7277 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add code to generate snapshots from preview configurations; Add SnapshottablePreviewConfigurations to Shared.
<!-- Thank you for submitting a Pull Request and helping to improve Home Assistant. Please complete the following sections to help the processing and review of your changes. Please do not delete anything from this template. --> ## Summary Put together a technique to reuse previews to generate and assert snapshots. This either saves time writing snapshot tests or makes snapshot tests more useful by using them as previews. This is implemented in a couple of libraries already ([example](https://github.com/BarredEwe/Prefire)). Went with a simple implementation here but a library can be adopted to leverage additional features and support. To hold the code for snapshotting, a new framework was introduced, SharedTesting, to collect all the utilities built for testing. It must only be imported in test targets. The preview side of the code lives in the Shared framework. Applying this new technique will come in a separate PR. [Original PR](#3484) --------- Co-authored-by: Bruno Pantaleão Gonçalves <5808343+bgoncal@users.noreply.github.com>
Remove build step to embed frameworks in SharedTesting framework.
…com/banaslee/homeassistant-iOS into snapshottable-preview-configuration
@bgoncal I reopened this PR. It contains just the application of the SnapshottablePreviews to LocationHistoryListView and Widgets, as well as the renaming of the snapshots (plus some deletions). This filter is still very useful to identify the relevant files to review https://github.com/home-assistant/iOS/pull/3484/files?file-filters%5B%5D=.pbxproj&file-filters%5B%5D=.swift&show-deleted-files=true&show-viewed-files=true |
Summary
SnapshottablePreviews applied in the LocationHistoryListView and Widgets.
In LocationHistoryListView, due to how Realm is used and how previews are implemented under the hood, the injected data is written both on
init
andonAppear
.For Widgets, not all the previews are visible. This is a limitation of the previews UI.
I also ended up removing the duplicate snapshots that would assert the behavior when more than the allowed tiles were passed. This can be restored if preferred.