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(profiling): add ui tests for new launch profiling modes #5008

Open
wants to merge 2 commits into
base: armcknight/profiling/new-continuous-apis/8-function-renames
Choose a base branch
from

Conversation

armcknight
Copy link
Member

with some small fixes needed in logic

also converted more bare asserts to warnings in production with early exits where needed.

#skip-changelog; for #4853

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

Attention: Patch coverage is 70.47619% with 31 lines in your changes missing coverage. Please review.

Project coverage is 92.571%. Comparing base (b6da0a4) to head (43fd159).
Report is 1 commits behind head on armcknight/profiling/new-continuous-apis/8-function-renames.

Files with missing lines Patch % Lines
...entry/Profiling/SentryProfiledTracerConcurrency.mm 54.054% 17 Missing ⚠️
Sources/Sentry/SentryProfiler.mm 72.727% 9 Missing ⚠️
Sources/Sentry/Profiling/SentryLaunchProfiling.m 84.615% 4 Missing ⚠️
...es/Sentry/Profiling/SentryProfilerSerialization.mm 50.000% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                                        Coverage Diff                                         @@
##           armcknight/profiling/new-continuous-apis/8-function-renames     #5008        +/-   ##
==================================================================================================
+ Coverage                                                        8.906%   92.571%   +83.665%     
==================================================================================================
  Files                                                              358       673       +315     
  Lines                                                            25668     82942     +57274     
  Branches                                                            94     30042     +29948     
==================================================================================================
+ Hits                                                              2286     76781     +74495     
+ Misses                                                           23382      6066     -17316     
- Partials                                                             0        95        +95     
Files with missing lines Coverage Δ
Sources/Sentry/SentryOptions.m 98.576% <100.000%> (+59.589%) ⬆️
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (+86.597%) ⬆️
Sources/Sentry/SentryTracer.m 97.163% <100.000%> (+42.825%) ⬆️
...es/Sentry/Profiling/SentryProfilerSerialization.mm 85.551% <50.000%> (+85.551%) ⬆️
Sources/Sentry/Profiling/SentryLaunchProfiling.m 89.312% <84.615%> (+89.312%) ⬆️
Sources/Sentry/SentryProfiler.mm 86.335% <72.727%> (+82.699%) ⬆️
...entry/Profiling/SentryProfiledTracerConcurrency.mm 89.787% <54.054%> (+80.017%) ⬆️

... and 658 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 b6da0a4...43fd159. Read the comment docs.

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

Copy link

github-actions bot commented Mar 21, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.54 ms 1249.45 ms 18.91 ms
Size 22.30 KiB 844.46 KiB 822.16 KiB

Baseline results on branch: armcknight/profiling/new-continuous-apis/8-function-renames

Startup times

Revision Plain With Sentry Diff
d974391 1241.08 ms 1258.67 ms 17.59 ms
006c78d 1226.40 ms 1241.45 ms 15.05 ms

App size

Revision Plain With Sentry Diff
d974391 22.30 KiB 843.77 KiB 821.47 KiB
006c78d 22.30 KiB 843.78 KiB 821.48 KiB

Previous results on branch: armcknight/profiling/new-continuous-apis/9-launch-profile-ui-tests

Startup times

Revision Plain With Sentry Diff
34a8741 1222.45 ms 1230.63 ms 8.18 ms
0eb5b93 1218.73 ms 1245.35 ms 26.61 ms

App size

Revision Plain With Sentry Diff
34a8741 22.30 KiB 844.48 KiB 822.18 KiB
0eb5b93 22.30 KiB 844.49 KiB 822.18 KiB

@armcknight armcknight force-pushed the armcknight/profiling/new-continuous-apis/8-function-renames branch from 80190da to b6da0a4 Compare March 21, 2025 08:34
@armcknight armcknight force-pushed the armcknight/profiling/new-continuous-apis/9-launch-profile-ui-tests branch from 3b90a13 to 43fd159 Compare March 21, 2025 08:34
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

Comment on lines +292 to +298
# if SENTRY_TEST || SENTRY_TESTCI
SENTRY_CASSERT(
NO, @"Expected a continuous profiler to be running for this configuration.");
# else
SENTRY_LOG_WARN(
@"Expected a continuous profiler to be running for this configuration.");
# endif // SENTRY_TEST || SENTRY_TESTCI
Copy link
Member

Choose a reason for hiding this comment

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

l: Maybe we could define a macro that calls SENTRY_CASSERT when SENTRY_TEST || SENTRY_TESTCI and calls SENTRY_LOG_WARN otherwise to reduce some duplication.

Comment on lines +113 to +114
case SentryProfilerTruncationReasonPanic:
return @"panic";
Copy link
Member

Choose a reason for hiding this comment

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

m: It would be great to have a test for this.

Comment on lines +708 to +713
- (BOOL)isProfilingCorrelatedToTraces
{
return ![self isContinuousProfilingEnabled]
|| _profiling.lifecycle == SentryProfileLifecycleTrace;
}

Copy link
Member

Choose a reason for hiding this comment

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

m: It would be great to have some unit tests for this logic.

…ames' into armcknight/profiling/new-continuous-apis/9-launch-profile-ui-tests
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