Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bugfix - send analytics even when BTAPIClient deallocated #1379
Bugfix - send analytics even when BTAPIClient deallocated #1379
Changes from 21 commits
0416807
3468a67
2da5b21
9df6e1d
cb2cf66
7b36004
7eb5c6e
593da6f
1ba1867
15f5faa
cb18f49
0515294
d296937
292bdb8
3a7c253
a101749
0d5bc74
b2715cd
fa0c3ac
2fb2cf2
cdc823f
71d6c0a
087d3a2
93c68e8
691cca3
217bdcd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
🫶🏻
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.
If you have a better name suggestion for this, please let me know
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.
Take it or leave it, but I know for other classes we have had the protocol conformance in an extension so we can add a
MARK:
to make it clear where the protocol starts/ends.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.
We could! Since the protocol is just for testing purposes, I thought that would make the functionality of BTAnalyticsService harder to read - but open to whatever folks prefer!
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.
sendAnalyticsEvent
andperformEventRequest
could be merged, or if we decide to keep both,performEventRequest
could be madeprivate
, thoughts?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.
We have those as 2 separate functions for the purpose of unit testing right now. Since
sendAnalyticsEvent
launches an async background task, there is no way for our tests to know when it completes to run it's assertions.performEventRequest
is acting as an "internal" version that delivers the async result, which we're calling in our unit tests.We used this pattern in PPCP too to unit test background-type tasks. Of course open to future suggestions here for improving our tests!
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.
I think
metadata
andauthorization
are constants in BTAPIClient (they are var for testing purposes). In the future, we might consider removing theBTAPIClient
dependency from here altogether and instead injectmetadata
,authorization
, andConfiguration
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.
Yeh this is a good callout. Though it did cross my mind going back to this PR (#1337) that we don't need a config to be present for analytics at the time of
BTAPIClient.sendAnalytics()
but rather right before we flush/batch upload the analytics after the 15 seconds timer.If we inject the Config from the BTAPIClient, we might get back to calling the
fetchOrReturnRemoteConfig
more times than necessaryThere 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.
:tears-of-joy: