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

test(profiling): more tests for continuous profiling #3985

Conversation

armcknight
Copy link
Member

Aiming to maximize code coverage of the code that's been added for continuous profiling, alongside the minimal tests added along the way.

for #3555; #skip-changelog

@armcknight armcknight changed the base branch from main to armcknight/feat/3555-continuous-profiling/5-implementation/6-move-timer-out-of-profiler-instance May 14, 2024 20:04
Copy link

github-actions bot commented May 14, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1204.45 ms 1228.67 ms 24.22 ms
Size 21.58 KiB 631.86 KiB 610.27 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c319795 1205.12 ms 1231.20 ms 26.08 ms
189b629 1211.16 ms 1224.30 ms 13.14 ms
60d6cec 1257.14 ms 1273.92 ms 16.78 ms
e79ead7 1216.91 ms 1247.06 ms 30.15 ms
e7b566f 1199.57 ms 1218.76 ms 19.19 ms
7fe37ab 1228.92 ms 1243.86 ms 14.94 ms
5616e0a 1237.00 ms 1260.43 ms 23.43 ms
d9308fd 1228.04 ms 1243.35 ms 15.31 ms
b2f82fa 1236.94 ms 1262.86 ms 25.92 ms
f2daa68 1238.40 ms 1256.43 ms 18.03 ms

App size

Revision Plain With Sentry Diff
c319795 20.76 KiB 431.99 KiB 411.22 KiB
189b629 20.76 KiB 399.69 KiB 378.93 KiB
60d6cec 22.84 KiB 403.51 KiB 380.67 KiB
e79ead7 20.76 KiB 426.11 KiB 405.35 KiB
e7b566f 22.85 KiB 414.80 KiB 391.95 KiB
7fe37ab 21.58 KiB 542.28 KiB 520.70 KiB
5616e0a 22.85 KiB 407.45 KiB 384.60 KiB
d9308fd 21.58 KiB 612.83 KiB 591.25 KiB
b2f82fa 20.76 KiB 419.62 KiB 398.86 KiB
f2daa68 21.58 KiB 418.40 KiB 396.82 KiB

Previous results on branch: armcknight/feat/3555-continuous-profiling/5-implementation/7-more-tests

Startup times

Revision Plain With Sentry Diff
008e90b 1242.47 ms 1260.23 ms 17.76 ms
00a035d 1215.40 ms 1231.21 ms 15.81 ms
f1a2b56 1227.57 ms 1237.54 ms 9.97 ms
d6e74c6 1233.02 ms 1246.87 ms 13.85 ms
ef8d366 1233.08 ms 1257.14 ms 24.06 ms

App size

Revision Plain With Sentry Diff
008e90b 21.58 KiB 631.84 KiB 610.26 KiB
00a035d 21.58 KiB 630.42 KiB 608.83 KiB
f1a2b56 21.58 KiB 631.84 KiB 610.25 KiB
d6e74c6 21.58 KiB 631.24 KiB 609.66 KiB
ef8d366 21.58 KiB 631.84 KiB 610.26 KiB

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

SentryTestUtils/Invocations.swift Outdated Show resolved Hide resolved
Base automatically changed from armcknight/feat/3555-continuous-profiling/5-implementation/6-move-timer-out-of-profiler-instance to main May 16, 2024 22:18
Copy link

codecov bot commented May 17, 2024

Codecov Report

Attention: Patch coverage is 99.26471% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 90.893%. Comparing base (a9ac2b2) to head (d239967).
Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3985       +/-   ##
=============================================
+ Coverage   90.857%   90.893%   +0.036%     
=============================================
  Files          594       594               
  Lines        46495     46593       +98     
  Branches     16652     16674       +22     
=============================================
+ Hits         42244     42350      +106     
+ Misses        4070      4061        -9     
- Partials       181       182        +1     
Files Coverage Δ
...urces/Sentry/Profiling/SentryContinuousProfiler.mm 93.506% <100.000%> (+3.633%) ⬆️
Sources/Sentry/SentryProfiler.mm 91.666% <100.000%> (+4.037%) ⬆️
...yProfilerTests/SentryAppLaunchProfilingTests.swift 100.000% <100.000%> (ø)
...yProfilerTests/SentryContinuousProfilerTests.swift 100.000% <100.000%> (ø)
...SentryProfilerTests/SentryProfileTestFixture.swift 99.408% <100.000%> (+0.007%) ⬆️
...SentryProfilerTests/SentryTraceProfilerTests.swift 98.352% <100.000%> (+0.027%) ⬆️
...urces/Sentry/Profiling/SentryProfilerTestHelpers.m 85.294% <94.117%> (+19.909%) ⬆️

... and 14 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 a9ac2b2...d239967. 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.

Again, LTGM, when the CI is green.

@armcknight armcknight force-pushed the armcknight/feat/3555-continuous-profiling/5-implementation/7-more-tests branch from ea32834 to b2d46e0 Compare May 17, 2024 19:01
@armcknight
Copy link
Member Author

I wound up simplifying the UI tests that integration test launch profiling. Since we now have an exhaustive check on all option combinations in unit tests, we don't need to replicate that again in the UI tests.

@armcknight armcknight merged commit b11cd87 into main May 21, 2024
69 of 70 checks passed
@armcknight armcknight deleted the armcknight/feat/3555-continuous-profiling/5-implementation/7-more-tests branch May 21, 2024 22:19
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