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

fix(feedback): accessibility audit action items #4698

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Jan 10, 2025

for #4401; #skip-changelog

Copy link

github-actions bot commented Jan 10, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1232.45 ms 1251.78 ms 19.33 ms
Size 22.31 KiB 818.61 KiB 796.29 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
65f104b 1229.87 ms 1247.69 ms 17.82 ms
46f5eb8 1212.27 ms 1231.42 ms 19.15 ms
0f30019 1239.22 ms 1240.04 ms 0.82 ms
c0ff306 1218.92 ms 1240.64 ms 21.72 ms
c021422 1237.12 ms 1263.18 ms 26.06 ms
bb5dc7d 1240.44 ms 1266.45 ms 26.01 ms
160a073 1260.72 ms 1270.10 ms 9.38 ms
9ae806a 1243.84 ms 1256.22 ms 12.39 ms
188547e 1241.47 ms 1257.35 ms 15.88 ms
156e771 1228.06 ms 1242.64 ms 14.58 ms

App size

Revision Plain With Sentry Diff
65f104b 21.58 KiB 625.82 KiB 604.24 KiB
46f5eb8 20.76 KiB 432.37 KiB 411.61 KiB
0f30019 22.84 KiB 405.39 KiB 382.54 KiB
c0ff306 20.76 KiB 434.65 KiB 413.89 KiB
c021422 20.76 KiB 435.64 KiB 414.88 KiB
bb5dc7d 22.85 KiB 412.98 KiB 390.13 KiB
160a073 22.85 KiB 408.88 KiB 386.03 KiB
9ae806a 21.58 KiB 616.14 KiB 594.56 KiB
188547e 21.58 KiB 424.34 KiB 402.76 KiB
156e771 20.76 KiB 419.70 KiB 398.94 KiB

Previous 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
0acedc3 1237.90 ms 1253.62 ms 15.72 ms
01ccbc7 1225.10 ms 1235.39 ms 10.29 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
0acedc3 22.31 KiB 780.06 KiB 757.75 KiB
01ccbc7 22.31 KiB 780.93 KiB 758.61 KiB
e3de806 22.31 KiB 770.93 KiB 748.62 KiB

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 69.72477% with 165 lines in your changes missing coverage. Please review.

Project coverage is 91.940%. Comparing base (f3b8999) to head (d6aecd5).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
...serFeedback/SentryUserFeedbackFormController.swift 41.614% 94 Missing ⚠️
...UserFeedback/SentryUserFeedbackFormViewModel.swift 82.448% 43 Missing ⚠️
...rFeedback/SentryUserFeedbackWidgetButtonView.swift 0.000% 14 Missing ⚠️
...Feedback/SentryUserFeedbackIntegrationDriver.swift 0.000% 5 Missing ⚠️
...ations/UserFeedback/SentryUserFeedbackWidget.swift 0.000% 5 Missing ⚠️
...ts/Integrations/Feedback/SentryFeedbackTests.swift 96.428% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4698       +/-   ##
=============================================
+ Coverage   91.082%   91.940%   +0.857%     
=============================================
  Files          624       627        +3     
  Lines        72205     72967      +762     
  Branches     25652     26560      +908     
=============================================
+ Hits         65766     67086     +1320     
+ Misses        6345      5782      -563     
- Partials        94        99        +5     
Files with missing lines Coverage Δ
Sources/Swift/Helper/Bundle.swift 100.000% <100.000%> (ø)
...guration/SentryUserFeedbackFormConfiguration.swift 100.000% <ø> (+100.000%) ⬆️
...ts/Integrations/Feedback/SentryFeedbackTests.swift 97.101% <96.428%> (-2.899%) ⬇️
...Feedback/SentryUserFeedbackIntegrationDriver.swift 0.000% <0.000%> (ø)
...ations/UserFeedback/SentryUserFeedbackWidget.swift 0.000% <0.000%> (ø)
...rFeedback/SentryUserFeedbackWidgetButtonView.swift 0.000% <0.000%> (ø)
...UserFeedback/SentryUserFeedbackFormViewModel.swift 89.523% <82.448%> (ø)
...serFeedback/SentryUserFeedbackFormController.swift 41.614% <41.614%> (ø)

... and 59 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 f3b8999...d6aecd5. Read the comment docs.

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.

I added a few comments.

Copy link
Contributor

@philprime philprime left a comment

Choose a reason for hiding this comment

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

LGTM, smaller comments to decide at your discretion

@@ -56,6 +56,10 @@ public class SentryUserFeedbackFormConfiguration: NSObject {
/**
* Allows the user to send a screenshot attachment with their feedback.
* - note: Default: `true`
* - warning: Your app must provide a value in its Info plist for the
Copy link
Contributor

Choose a reason for hiding this comment

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

l: do we still need this warning?

@@ -30,4 +55,107 @@ class SentryFeedbackTests: XCTestCase {
let attachments = sut.attachments()
XCTAssertEqual(attachments.count, 0)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

l: instead of creating a single test which loops over all input combinations, you could create individual tests for each input combination, but each one calls a helper function. The helper function would then pass ...file: StaticString = #file, line: UInt = #line) as parameters down to the XCTAssertEqual(..., file: file, line: line), therefore propagating the error up to the calling function.

That would then result in individual tests failing if the input combination is not working, rather than failing the whole

@@ -1 +1 @@
19.1.6
19.1.7
Copy link
Contributor

Choose a reason for hiding this comment

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

l: maybe move this and the swiftlint-version to its own PR

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