Skip to content

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Jun 23, 2025

Replace stop(gracefully) with removeObservers.

This PR is based on #5121 (comment).

#skip-changelog

// After stopping the session tracker, we need to set a new hub to the SDK.
fixture.setNewHubToSDK()
}

private func abnormalStopSut() {
sut.stop(withGracefully: false)
sut.removeObservers()
Copy link
Member

Choose a reason for hiding this comment

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

h: We still need to reset the wasStartSessionCalled, that's why I didn't just remove the observer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. The tests are all green, cause they all create a new sut. You only need to set wasStartSessionCalled = NO in stop, when you're not getting a new instance.

Copy link
Member

Choose a reason for hiding this comment

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

ok if tests are green, LGTM.

Copy link

codecov bot commented Jun 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.086%. Comparing base (cd95f33) to head (6fa8d8a).
Report is 1 commits behind head on philprime/issue-5069.

Additional details and impacted files

Impacted file tree graph

@@                    Coverage Diff                     @@
##           philprime/issue-5069     #5461       +/-   ##
==========================================================
- Coverage                86.214%   86.086%   -0.128%     
==========================================================
  Files                       399       399               
  Lines                     34775     34765       -10     
  Branches                  15078     14882      -196     
==========================================================
- Hits                      29981     29928       -53     
- Misses                     4750      4797       +47     
+ Partials                     44        40        -4     
Files with missing lines Coverage Δ
...rces/Sentry/SentryAutoSessionTrackingIntegration.m 100.000% <100.000%> (ø)
Sources/Sentry/SentrySessionTracker.m 99.152% <100.000%> (+0.007%) ⬆️

... and 19 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 cd95f33...6fa8d8a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@philipphofmann philipphofmann merged commit d5b55b1 into philprime/issue-5069 Jun 23, 2025
124 of 128 checks passed
@philipphofmann philipphofmann deleted the ref/philipp-remove-observers branch June 23, 2025 08:58
Copy link
Contributor

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1244.35 ms 1264.53 ms 20.18 ms
Size 23.75 KiB 848.13 KiB 824.38 KiB

Baseline results on branch: philprime/issue-5069

Startup times

Revision Plain With Sentry Diff
927261d 1210.24 ms 1238.59 ms 28.35 ms
b5d1c3d 1231.00 ms 1255.47 ms 24.47 ms
bc06793 1231.04 ms 1247.98 ms 16.94 ms
4deddc5 1220.81 ms 1234.41 ms 13.60 ms
6996d49 1213.72 ms 1231.27 ms 17.55 ms
334d6f2 1202.65 ms 1232.21 ms 29.56 ms
5be643e 1217.51 ms 1246.15 ms 28.64 ms
7ba9cd4 1227.26 ms 1258.63 ms 31.37 ms
825200b 1200.16 ms 1227.76 ms 27.59 ms
9c7c623 1231.18 ms 1249.28 ms 18.09 ms

App size

Revision Plain With Sentry Diff
927261d 23.75 KiB 843.32 KiB 819.57 KiB
b5d1c3d 23.75 KiB 841.30 KiB 817.55 KiB
bc06793 23.75 KiB 847.75 KiB 824.00 KiB
4deddc5 23.76 KiB 822.08 KiB 798.32 KiB
6996d49 23.75 KiB 847.75 KiB 824.00 KiB
334d6f2 23.75 KiB 838.54 KiB 814.79 KiB
5be643e 23.76 KiB 820.06 KiB 796.30 KiB
7ba9cd4 23.75 KiB 847.86 KiB 824.12 KiB
825200b 23.75 KiB 847.90 KiB 824.15 KiB
9c7c623 23.76 KiB 822.00 KiB 798.24 KiB

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.

2 participants