Skip to content

test: Add tests for CoreDataCrash #3165

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

Merged
merged 7 commits into from
Jul 18, 2023
Merged

Conversation

philipphofmann
Copy link
Member

Add a changelog entry and some tests for #3152.

#skip-changelog

Add a changelog entry and some tests for #3152.
@philipphofmann philipphofmann changed the title test: Add test for CoreDataCrash test: Add tests for CoreDataCrash Jul 17, 2023
@philipphofmann philipphofmann enabled auto-merge (squash) July 17, 2023 07:52
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #3165 (844c713) into main (27cd119) will increase coverage by 0.004%.
The diff coverage is 97.402%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3165       +/-   ##
=============================================
+ Coverage   89.094%   89.098%   +0.004%     
=============================================
  Files          502       503        +1     
  Lines        53908     53965       +57     
  Branches     19312     19339       +27     
=============================================
+ Hits         48029     48082       +53     
- Misses        5022      5026        +4     
  Partials       857       857               
Impacted Files Coverage Δ
...ance/CoreData/SentryCoreDataTrackerExtension.swift 90.909% <90.909%> (ø)
Sources/Sentry/SentryCoreDataSwizzling.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryCoreDataTrackingIntegration.m 100.000% <100.000%> (ø)
.../Performance/CoreData/SentryCoreDataTracker+Test.m 100.000% <100.000%> (ø)
...rformance/CoreData/SentryCoreDataTrackerTest.swift 91.317% <100.000%> (+0.763%) ⬆️
...reData/SentryCoreDataTrackingIntegrationTest.swift 100.000% <100.000%> (ø)

... and 4 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 27cd119...844c713. Read the comment docs.

@@ -2,12 +2,20 @@ import Foundation

extension SentryCoreDataMiddleware {

func fetchManagedObjectContext<T>(_ context: NSManagedObjectContext, request: NSFetchRequest<T>, originalImp: (NSFetchRequest<T>, NSErrorPointer) -> [T]?) throws -> [Any] {
func fetchManagedObjectContext<T>(_ context: NSManagedObjectContext, request: NSFetchRequest<T>, isErrorNil: Bool = false, originalImp: (NSFetchRequest<T>, NSErrorPointer) -> [T]?) throws -> [Any] {
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused on how the CoreDataTracker, middleware and swizzling work. Are the methods in @protocol SentryCoreDataMiddleware supposed to be called by SDK consumers, or are they called somewhere else by us in the SDK code? SentryCoreDataSwizzling.h is not a public header, so it must be coming from the SDK somewhere.

What I'm searching for is where the inout NSError actually originates.

I think we should add headerdocs to SentryCoreDataSwizzling.h to explain how it's supposed to work.

Copy link
Member

Choose a reason for hiding this comment

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

I think I get it now, we swizzle -[NSManagedObjectContext executeFetchRequest:error:] et al and the protocol methods are substituted for the originals.

Seeing as how there is only one class conforming to SentryCoreDataMiddleware I'm not convinced we even need that protocol. I think I brought this up previously when it was being implemented. It's overly complicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was also confused. I can improve it a bit.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for doing that! 🙏🏻

@github-actions
Copy link
Contributor

github-actions bot commented Jul 18, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1218.86 ms 1244.52 ms 25.66 ms
Size 22.84 KiB 403.17 KiB 380.32 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
fc163f5 1198.05 ms 1227.76 ms 29.71 ms
1bbcb9c 1192.51 ms 1231.96 ms 39.45 ms
c0b4b71 1246.98 ms 1256.77 ms 9.79 ms
90d17d3 1261.18 ms 1278.18 ms 17.00 ms
eed479f 1198.93 ms 1228.12 ms 29.19 ms
ff09c7e 1244.86 ms 1246.68 ms 1.82 ms
154f795 1250.38 ms 1274.54 ms 24.16 ms
369222e 1232.14 ms 1258.90 ms 26.76 ms
596ccc1 1221.57 ms 1236.82 ms 15.25 ms
591a01b 1197.94 ms 1222.53 ms 24.59 ms

App size

Revision Plain With Sentry Diff
fc163f5 20.76 KiB 436.30 KiB 415.54 KiB
1bbcb9c 20.76 KiB 426.10 KiB 405.34 KiB
c0b4b71 20.76 KiB 430.98 KiB 410.22 KiB
90d17d3 20.76 KiB 432.17 KiB 411.41 KiB
eed479f 20.76 KiB 433.18 KiB 412.42 KiB
ff09c7e 20.76 KiB 427.77 KiB 407.00 KiB
154f795 20.76 KiB 435.25 KiB 414.49 KiB
369222e 20.76 KiB 419.67 KiB 398.91 KiB
596ccc1 22.84 KiB 401.44 KiB 378.60 KiB
591a01b 22.84 KiB 401.67 KiB 378.83 KiB

Previous results on branch: test/for-core-data-crash

Startup times

Revision Plain With Sentry Diff
e891602 1238.02 ms 1251.38 ms 13.36 ms
390efa3 1238.30 ms 1263.60 ms 25.30 ms

App size

Revision Plain With Sentry Diff
e891602 22.84 KiB 403.27 KiB 380.43 KiB
390efa3 22.84 KiB 403.28 KiB 380.43 KiB

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the improvements 👍🏻

@philipphofmann philipphofmann merged commit 4946815 into main Jul 18, 2023
@philipphofmann philipphofmann deleted the test/for-core-data-crash branch July 18, 2023 19:20
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