segment-integrations / analytics-ios-integration-firebase

Segment's bundled integration for Firebase on iOS
MIT License
13 stars 91 forks source link

Skip FIRApp configure when testing #102

Closed gngrwzrd closed 1 year ago

gngrwzrd commented 1 year ago

I recently added the SEGFirebase integration to an app. It works great! but we are experiencing crashes during our tests that have been hard to come to a real conclusion about.

Our app checks [FIRApp defaultApp] before calling [FIRApp configure] - just as your framework does. But we still run into crashes during initialization of the SEGFirebaseIntegration class.

I wish I had more to demonstrate other than a screenshot.

We only receive this crash when we are running tests in CI on Bitrise and Xcode Cloud which is the reason this has been hard to get a stack trace or anything more valuable.

What this PR does:

Screenshot 2023-06-27 at 2 29 05 PM
gngrwzrd commented 1 year ago

I've tested in our unit tests by patching the source files in the pod. And this fixes it.

If you had other suggestions for how to debug I'm all ears.

gngrwzrd commented 1 year ago

@bsneed Hey! Remember we briefly worked on the Gity app together! 👋🏻

bsneed commented 1 year ago

@gngrwzrd Hey hey Aaron! Long time no see! Hope you've been well brother!

I think you might be seeing a race condition. If you could share your setup of the segment lib, and as well as where you're calling FIRApp yourself, we might be able to narrow it down.

Also, these libs (analytics-ios, analytics-ios-integration-firebase, etc) are on the first of being EOL'd. Is there any possibility to get you over to analytics-swift? The API is largely compatible outside of the startup bit.

gngrwzrd commented 1 year ago

@bsneed 👋🏻 No problem.

Oh that's interesting.. When I google search segment iOS it takes me to this Quickstart.(https://segment.com/docs/connections/sources/catalog/libraries/mobile/ios/quickstart/). But I did find the Analytics-Swift docs now. Y'all should include some kind of notice on this documentation to go to Analytics-Swift or redirect.

Are you forcing SPM support? Why? I've looked at migrating to SPM but a few of our other dependencies don't support it yet. We store Pods in our repo; pods are a build time optimization for us so build machines don't need to download anything.

Related to this PR though. I found another solution. I'll just skip adding the integration when we're running unit tests. That way the framework isn't called. Sorry for the waste of time here!

This PR can happily be closed otherwise!

bsneed commented 1 year ago

@gngrwzrd Yeah, we're working on getting docs all sorted out on our path to deprecating those. I'll relay your comment as a reminder and kick in the pants to the powers that be to get on with it. :D

Yeah, we're only supporting SPM going forward. You should be able to do the same thing with the SPM packages, the overall structure would just be a little different I think. Re not supporting Cocoapods in general, they seem to have a hard time keeping up with Apple's tooling changes, much like Carthage, which we also dropped support for. The fortunate thing is that it's relatively simple to have a fork w/ a pod spec in it. You can see a few examples by looking at the forks graph.