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: Remove "/" from crash report file name #3005

Merged
merged 8 commits into from May 8, 2023
Merged

Conversation

brustolin
Copy link
Contributor

📜 Description

Crash report uses bundleName to create the report file, but "/" is a valid character for bundle name but not for file name.
Replace "/" with "-" for the report file name.

💡 Motivation and Context

Close #2975

💚 How did you test it?

Unit test and sample.

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • 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.
  • 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

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #3005 (da7e6d4) into main (2b4a663) will decrease coverage by 0.023%.
The diff coverage is 75.862%.

❗ Current head da7e6d4 differs from pull request most recent head 7223cbe. Consider uploading reports for the commit 7223cbe to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3005       +/-   ##
=============================================
- Coverage   88.371%   88.348%   -0.023%     
=============================================
  Files          495       495               
  Lines        53264     53231       -33     
  Branches     19141     19114       -27     
=============================================
- Hits         47070     47029       -41     
- Misses        5237      5240        +3     
- Partials       957       962        +5     
Impacted Files Coverage Δ
Sources/SentryCrash/Recording/SentryCrash.m 76.035% <70.833%> (+0.295%) ⬆️
Tests/SentryTests/SentryCrash/SentryCrashTests.m 100.000% <100.000%> (ø)

... and 16 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 2b4a663...7223cbe. Read the comment docs.

@github-actions
Copy link

github-actions bot commented May 5, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1238.24 ms 1255.74 ms 17.50 ms
Size 20.76 KiB 437.12 KiB 416.36 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
326b7eb 1252.86 ms 1259.56 ms 6.70 ms
c6504da 1232.06 ms 1243.28 ms 11.22 ms
d3abae0 1200.36 ms 1224.22 ms 23.87 ms
630ddf4 1216.50 ms 1235.94 ms 19.44 ms
5de0a56 1214.49 ms 1235.56 ms 21.07 ms
407ff99 1216.63 ms 1235.50 ms 18.87 ms
005bb4c 1237.38 ms 1255.54 ms 18.16 ms
7fb7afb 1230.12 ms 1251.04 ms 20.92 ms
cf724da 1243.14 ms 1261.44 ms 18.30 ms
3f1be0f 1208.12 ms 1225.72 ms 17.60 ms

App size

Revision Plain With Sentry Diff
326b7eb 20.76 KiB 432.31 KiB 411.55 KiB
c6504da 20.76 KiB 414.44 KiB 393.69 KiB
d3abae0 20.76 KiB 434.92 KiB 414.16 KiB
630ddf4 20.76 KiB 432.37 KiB 411.61 KiB
5de0a56 20.76 KiB 432.87 KiB 412.11 KiB
407ff99 20.76 KiB 427.87 KiB 407.10 KiB
005bb4c 20.76 KiB 419.70 KiB 398.94 KiB
7fb7afb 20.76 KiB 419.70 KiB 398.94 KiB
cf724da 20.76 KiB 430.52 KiB 409.76 KiB
3f1be0f 20.76 KiB 414.44 KiB 393.69 KiB

Previous results on branch: fix/dash-in-the-name

Startup times

Revision Plain With Sentry Diff
7aa6482 1241.02 ms 1253.20 ms 12.18 ms
f7b52e9 1209.29 ms 1233.04 ms 23.75 ms

App size

Revision Plain With Sentry Diff
7aa6482 20.76 KiB 437.12 KiB 416.36 KiB
f7b52e9 20.76 KiB 436.82 KiB 416.06 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.

Thanks, @brustolin.

Sources/SentryCrash/Recording/SentryCrash.m Show resolved Hide resolved
@armcknight
Copy link
Member

armcknight commented May 8, 2023

Edit: I got mixed up, this is for bundle name, not bundle identifier.

@brustolin brustolin merged commit c791a49 into main May 8, 2023
@brustolin brustolin deleted the fix/dash-in-the-name branch May 8, 2023 15:50
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.

No automatic Crash upload
5 participants