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

Snapshot preview reuse #3484

Merged
merged 51 commits into from
Mar 24, 2025
Merged

Conversation

banaslee
Copy link
Contributor

@banaslee banaslee commented Mar 17, 2025

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 and onAppear.

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.

banaslee and others added 27 commits February 27, 2025 23:30
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
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
@bgoncal
Copy link
Member

bgoncal commented Mar 18, 2025

Can you split this into more than 1 PR? One for the sharing new target, one for location history and one for widgets.
Just to be easier to review, but if it endup being hard it's fine, I'll just take more time to review since for these changes I'll have to checkout the branch.
Github is not even loading the snapshot diffs to me

@banaslee
Copy link
Contributor Author

Can you split this into more than 1 PR? One for the sharing new target, one for location history and one for widgets. Just to be easier to review, but if it endup being hard it's fine, I'll just take more time to review since for these changes I'll have to checkout the branch. Github is not even loading the snapshot diffs to me

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.

@bgoncal
Copy link
Member

bgoncal commented Mar 18, 2025

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.

@banaslee
Copy link
Contributor Author

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.

Copy link

codecov bot commented Mar 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.51%. Comparing base (a56a0b7) to head (5689ddd).
Report is 2 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Add code to generate snapshots from preview configurations;
Add SnapshottablePreviewConfigurations to Shared.
@banaslee banaslee marked this pull request as draft March 19, 2025 00:41
bgoncal added a commit that referenced this pull request Mar 20, 2025
<!-- 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>
@banaslee banaslee marked this pull request as ready for review March 21, 2025 15:21
@banaslee
Copy link
Contributor Author

banaslee commented Mar 21, 2025

@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

@bgoncal bgoncal merged commit d7924bc into home-assistant:master Mar 24, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants