-
-
Notifications
You must be signed in to change notification settings - Fork 342
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
base: armcknight/profiling/new-continuous-apis/8-function-renames
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
... and 658 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Performance metrics 🚀
|
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 |
…ogic in some places
80190da
to
b6da0a4
Compare
3b90a13
to
43fd159
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
# 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 |
There was a problem hiding this comment.
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.
case SentryProfilerTruncationReasonPanic: | ||
return @"panic"; |
There was a problem hiding this comment.
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.
- (BOOL)isProfilingCorrelatedToTraces | ||
{ | ||
return ![self isContinuousProfilingEnabled] | ||
|| _profiling.lifecycle == SentryProfileLifecycleTrace; | ||
} | ||
|
There was a problem hiding this comment.
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
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