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

refactor: Change macro TEST and TESTCI to SENTRY_TEST and SENTRY_TEST_CI #4712

Merged
merged 5 commits into from
Jan 15, 2025

Conversation

philprime
Copy link
Contributor

📜 Description

Replaced all occurences of the macros TEST and TESTCI with SENTRY_TEST and SENTRY_TEST_CI

💡 Motivation and Context

Closes #4638

💚 How did you test it?

CI checks

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Copy link

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNSDataSwizzling.m
  • Sources/Sentry/SentryNSFileManagerSwizzling.m
  • Sources/Sentry/SentrySwizzle.m

Copy link

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNSDataSwizzling.m
  • Sources/Sentry/SentryNSFileManagerSwizzling.m
  • Sources/Sentry/SentrySwizzle.m

Copy link

github-actions bot commented Jan 14, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1237.84 ms 1257.65 ms 19.82 ms
Size 22.31 KiB 768.52 KiB 746.21 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
cce35c6 1228.88 ms 1241.14 ms 12.26 ms
522f8da 1230.19 ms 1252.31 ms 22.12 ms
2154ae1 1216.06 ms 1244.78 ms 28.72 ms
132ea11 1224.45 ms 1254.47 ms 30.02 ms
3f366ee 1244.49 ms 1257.28 ms 12.79 ms
44ce888 1208.98 ms 1224.72 ms 15.74 ms
3db3e35 1233.52 ms 1256.43 ms 22.90 ms
973d574 1237.35 ms 1256.15 ms 18.80 ms
b15521e 1231.82 ms 1252.23 ms 20.41 ms
4b22fc2 1192.15 ms 1217.31 ms 25.16 ms

App size

Revision Plain With Sentry Diff
cce35c6 20.76 KiB 425.80 KiB 405.04 KiB
522f8da 21.90 KiB 726.18 KiB 704.28 KiB
2154ae1 20.76 KiB 427.83 KiB 407.06 KiB
132ea11 21.90 KiB 708.95 KiB 687.05 KiB
3f366ee 20.76 KiB 427.84 KiB 407.08 KiB
44ce888 22.85 KiB 414.65 KiB 391.80 KiB
3db3e35 21.58 KiB 419.21 KiB 397.63 KiB
973d574 21.58 KiB 542.39 KiB 520.81 KiB
b15521e 21.58 KiB 573.18 KiB 551.59 KiB
4b22fc2 22.85 KiB 414.11 KiB 391.26 KiB

Previous results on branch: philprime/issue-4638-test-macro-replacement

Startup times

Revision Plain With Sentry Diff
8bc7f4f 1226.51 ms 1249.13 ms 22.61 ms
33ed280 1221.23 ms 1243.82 ms 22.58 ms

App size

Revision Plain With Sentry Diff
8bc7f4f 22.31 KiB 768.53 KiB 746.21 KiB
33ed280 22.31 KiB 768.52 KiB 746.21 KiB

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.

This is a nice thing to have, just need to fix CI.

Copy link

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNSDataSwizzling.m
  • Sources/Sentry/SentryNSFileManagerSwizzling.m
  • Sources/Sentry/SentrySwizzle.m

Copy link

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNSDataSwizzling.m
  • Sources/Sentry/SentryNSFileManagerSwizzling.m
  • Sources/Sentry/SentrySwizzle.m

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 92.10526% with 3 lines in your changes missing coverage. Please review.

Project coverage is 91.102%. Comparing base (72e34fa) to head (2ccd0de).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...grations/UserFeedback/SentryUserFeedbackForm.swift 0.000% 2 Missing ⚠️
Sources/Sentry/SentryProfiler.mm 50.000% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4712       +/-   ##
=============================================
- Coverage   91.103%   91.102%   -0.001%     
=============================================
  Files          624       624               
  Lines        72085     72090        +5     
  Branches     25595     25611       +16     
=============================================
+ Hits         65672     65676        +4     
  Misses        6319      6319               
- Partials        94        95        +1     
Files with missing lines Coverage Δ
...urces/Sentry/Profiling/SentryContinuousProfiler.mm 92.741% <ø> (ø)
Sources/Sentry/Profiling/SentryLaunchProfiling.m 90.983% <ø> (ø)
...entry/Profiling/SentryProfiledTracerConcurrency.mm 98.734% <ø> (ø)
...es/Sentry/Profiling/SentryProfilerSerialization.mm 85.769% <100.000%> (ø)
...urces/Sentry/Profiling/SentryProfilerTestHelpers.m 87.804% <ø> (ø)
Sources/Sentry/Profiling/SentryTraceProfiler.mm 95.522% <ø> (ø)
Sources/Sentry/SentryAppStartTracker.m 98.224% <100.000%> (ø)
Sources/Sentry/SentryBreadcrumbTracker.m 87.623% <100.000%> (ø)
Sources/Sentry/SentryEnvelope.m 89.320% <100.000%> (ø)
Sources/Sentry/SentryExtraPackages.m 95.652% <ø> (ø)
... and 22 more

... and 9 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 72e34fa...2ccd0de. 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.

Thanks, please merge this quickly to avoid merge conflicts.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Philipp Hofmann <[email protected]>
Copy link

🚨 Detected changes in high risk code 🚨

High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:

  • Sources/Sentry/SentryNSDataSwizzling.m
  • Sources/Sentry/SentryNSFileManagerSwizzling.m
  • Sources/Sentry/SentrySwizzle.m

@philprime philprime merged commit f1e2aa7 into main Jan 15, 2025
46 of 48 checks passed
@philprime philprime deleted the philprime/issue-4638-test-macro-replacement branch January 15, 2025 09:45
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.

[Potential Risk] Please consider renaming the TEST macro
3 participants