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

ref(feedback): split up SentryUserFeedbackForm.swift #4726

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Jan 20, 2025

so that we can unit test some things. includes a couple fixes along the way (which might be better served moving to #4727). follows #4698, in particular, #4698 (comment)

  • rename SentryUserFeedbackForm.swift to SentryUserFeedbackFormController.swift
  • split out SentryUserFeedbackFormViewModel.swift to define the UI elements, layout stuff, and some derived values in pure functions without stateful side effects in the view controller
  • (I reverted this because I didn't want to have to ship TestSentryPhotoPicker in the SDK code itself to be able to use from the UI tests) split out the photo picker to make it cleaner to substitute in a mock (although we still have to do it in the production code for UI tests)
  • added exhaustive unit test of all state combinations that lead to the computation of the accessibility hint for the submit button
  • this implicitly tests the validation logic, which was combined more closely with the accessibility hint generation into a function that now returns both results in a tuple Result<String, Error>; this way we don't have two parallel implementations that can diverge and disagree at any point (and some of the code already was trying to handle that...)

TODO

  • fix the remaining broken unit test cases after the changes to submit button accessibility hints

#skip-changelog

@armcknight armcknight changed the title ref: split out a model view from the main controller class ref(feedback): split out a model view from the main controller class Jan 20, 2025
@armcknight armcknight changed the title ref(feedback): split out a model view from the main controller class ref(feedback): split up SentryUserFeedbackForm.swift Jan 20, 2025
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 71.57676% with 137 lines in your changes missing coverage. Please review.

Project coverage is 91.895%. Comparing base (a2d60f2) to head (7d710e0).
Report is 1 commits behind head on armcknight/feat(feedback)/accessibility-audit.

Files with missing lines Patch % Lines
...serFeedback/SentryUserFeedbackFormController.swift 38.775% 90 Missing ⚠️
...UserFeedback/SentryUserFeedbackFormViewModel.swift 81.081% 42 Missing ⚠️
...ts/Integrations/Feedback/SentryFeedbackTests.swift 96.428% 4 Missing ⚠️
...ations/UserFeedback/SentryUserFeedbackWidget.swift 0.000% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                                 Coverage Diff                                 @@
##           armcknight/feat(feedback)/accessibility-audit     #4726       +/-   ##
===================================================================================
+ Coverage                                         91.022%   91.895%   +0.873%     
===================================================================================
  Files                                                624       625        +1     
  Lines                                              72322     72475      +153     
  Branches                                           26307     26301        -6     
===================================================================================
+ Hits                                               65829     66601      +772     
+ Misses                                              6395      5774      -621     
- Partials                                              98       100        +2     
Files with missing lines Coverage Δ
...ations/UserFeedback/SentryUserFeedbackWidget.swift 0.000% <0.000%> (ø)
...ts/Integrations/Feedback/SentryFeedbackTests.swift 97.101% <96.428%> (-2.899%) ⬇️
...UserFeedback/SentryUserFeedbackFormViewModel.swift 89.638% <81.081%> (ø)
...serFeedback/SentryUserFeedbackFormController.swift 38.775% <38.775%> (ø)

... and 31 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2d60f2...7d710e0. Read the comment docs.

Copy link

github-actions bot commented Jan 20, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1209.19 ms 1219.25 ms 10.06 ms
Size 22.31 KiB 775.97 KiB 753.66 KiB

Baseline results on branch: armcknight/feat(feedback)/accessibility-audit

Startup times

Revision Plain With Sentry Diff
3cbcd11 1220.02 ms 1242.74 ms 22.72 ms
e3de806 1242.67 ms 1262.22 ms 19.56 ms

App size

Revision Plain With Sentry Diff
3cbcd11 22.31 KiB 774.49 KiB 752.17 KiB
e3de806 22.31 KiB 770.93 KiB 748.62 KiB

Previous results on branch: armcknight/armcknight/feat(feedback)/form-controller-refactor

Startup times

Revision Plain With Sentry Diff
135f1cc 1228.78 ms 1244.12 ms 15.35 ms
6a01098 1236.08 ms 1251.76 ms 15.67 ms
2f59cde 1221.22 ms 1243.70 ms 22.48 ms
484eb3c 1222.22 ms 1234.35 ms 12.12 ms
031bb71 1229.60 ms 1248.00 ms 18.40 ms

App size

Revision Plain With Sentry Diff
135f1cc 22.31 KiB 777.51 KiB 755.19 KiB
6a01098 22.31 KiB 775.97 KiB 753.66 KiB
2f59cde 22.31 KiB 777.38 KiB 755.07 KiB
484eb3c 22.31 KiB 776.35 KiB 754.04 KiB
031bb71 22.31 KiB 775.98 KiB 753.66 KiB

@armcknight armcknight marked this pull request as ready for review January 28, 2025 00:33
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, if #4726 (comment) is addressed in another PR.

@armcknight armcknight merged commit a99c302 into armcknight/feat(feedback)/accessibility-audit Jan 28, 2025
64 of 68 checks passed
@armcknight armcknight deleted the armcknight/armcknight/feat(feedback)/form-controller-refactor branch January 28, 2025 22:00
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.

4 participants