Skip to content

Conversation

@armcknight
Copy link
Member

Same deal, move the singleton to SentryDependencyContainer.

After making this change, SentryCrashInstallationTests.testUninstall fails when running the full test suite locally, but not when run by itself. Seems like there's a bug in the tests. Haven't sussed it out yet though.

#skip-changelog

@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #3247 (4e82b5b) into main (e5dcbd5) will increase coverage by 0.045%.
The diff coverage is 97.916%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3247       +/-   ##
=============================================
+ Coverage   88.988%   89.033%   +0.045%     
=============================================
  Files          521       521               
  Lines        55869     56045      +176     
  Branches     19868     20168      +300     
=============================================
+ Hits         49717     49899      +182     
+ Misses        5232      5227        -5     
+ Partials       920       919        -1     
Files Coverage Δ
SentryTestUtils/ClearTestState.swift 100.000% <100.000%> (ø)
Sources/Sentry/SentryCrashIntegration.m 94.771% <100.000%> (ø)
Sources/Sentry/SentryDependencyContainer.m 95.141% <100.000%> (+0.204%) ⬆️
Sources/Sentry/SentrySDK.m 89.787% <100.000%> (ø)
...entryCrash/Installations/SentryCrashInstallation.m 32.432% <100.000%> (ø)
...ording/Monitors/SentryCrashMonitor_MachException.c 35.051% <100.000%> (ø)
...ecording/Monitors/SentryCrashMonitor_NSException.m 33.823% <100.000%> (+0.987%) ⬆️
Sources/SentryCrash/Recording/SentryCrash.m 75.675% <ø> (-0.431%) ⬇️
...ions/SentryCrash/SentryCrashIntegrationTests.swift 84.498% <100.000%> (+0.559%) ⬆️
...egrations/SentryCrash/SentryCrashReportTests.swift 90.789% <100.000%> (ø)
... and 5 more

... and 69 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 e5dcbd5...4e82b5b. Read the comment docs.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 19, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1243.20 ms 1252.34 ms 9.14 ms
Size 22.85 KiB 413.89 KiB 391.04 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
279841c 1231.41 ms 1245.65 ms 14.24 ms
0ffe199 1209.51 ms 1223.94 ms 14.43 ms
368187c 1220.00 ms 1247.12 ms 27.12 ms
ce4cfaf 1203.61 ms 1218.86 ms 15.25 ms
6001822 1265.10 ms 1270.40 ms 5.30 ms
1bf8571 1250.96 ms 1255.36 ms 4.40 ms
ae9c51b 1267.73 ms 1272.33 ms 4.59 ms
1db04d8 1250.20 ms 1258.12 ms 7.92 ms
7ce3cf6 1217.98 ms 1246.41 ms 28.43 ms
25a5e8b 1235.16 ms 1255.47 ms 20.30 ms

App size

Revision Plain With Sentry Diff
279841c 22.84 KiB 403.19 KiB 380.35 KiB
0ffe199 20.76 KiB 436.50 KiB 415.74 KiB
368187c 20.76 KiB 436.50 KiB 415.74 KiB
ce4cfaf 20.76 KiB 423.19 KiB 402.43 KiB
6001822 22.85 KiB 410.98 KiB 388.13 KiB
1bf8571 20.76 KiB 437.12 KiB 416.36 KiB
ae9c51b 22.85 KiB 411.13 KiB 388.28 KiB
1db04d8 20.76 KiB 435.50 KiB 414.74 KiB
7ce3cf6 22.85 KiB 407.63 KiB 384.78 KiB
25a5e8b 20.76 KiB 436.33 KiB 415.57 KiB

Previous results on branch: armcknight/ref/centralize-SentryCrash-access

Startup times

Revision Plain With Sentry Diff
bf1c527 1199.36 ms 1233.10 ms 33.74 ms
6269a76 1209.55 ms 1236.54 ms 26.99 ms
b1b3103 1213.32 ms 1232.52 ms 19.20 ms
bd41304 1230.38 ms 1233.84 ms 3.46 ms
a4eeb2d 1234.04 ms 1249.84 ms 15.80 ms

App size

Revision Plain With Sentry Diff
bf1c527 22.85 KiB 405.55 KiB 382.70 KiB
6269a76 22.85 KiB 407.78 KiB 384.93 KiB
b1b3103 22.85 KiB 405.99 KiB 383.14 KiB
bd41304 22.85 KiB 413.89 KiB 391.04 KiB
a4eeb2d 22.85 KiB 407.22 KiB 384.37 KiB

@brustolin
Copy link
Contributor

Please fix CI.

@brustolin
Copy link
Contributor

We need CI fixed.

@getsantry
Copy link

getsantry bot commented Oct 18, 2023

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Oct 18, 2023
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM.

Just make sure CI is working properly before merging this.

@armcknight armcknight removed the Stale label Oct 25, 2023
@armcknight
Copy link
Member Author

Still not sure why the test is failing for tvOS to test uninstalling the crash handler. But, I was unable to even run it locally, which #3382 should fix. It could be related to dirty state between tests, which #3381 attempts to fix more.

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.

Thanks @armcknight 👍

@armcknight
Copy link
Member Author

Found it: SentryCrashInstallationReporterTests never uninstalled the crash reporters it installed.

@armcknight armcknight merged commit c471221 into main Nov 7, 2023
@armcknight armcknight deleted the armcknight/ref/centralize-SentryCrash-access branch November 7, 2023 22:50
@armcknight armcknight mentioned this pull request Nov 8, 2023
7 tasks
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