Perform product entitlement mapping request after more critical requests#1017
Conversation
There was a problem hiding this comment.
This one wasn't really needed since onAppForeground is also executed on initial SDK configuration, even if it's AFTER the app is open. We also request offerings from there, which is much more important.
Codecov Report
@@ Coverage Diff @@
## main #1017 +/- ##
==========================================
- Coverage 85.57% 85.32% -0.25%
==========================================
Files 171 169 -2
Lines 6067 6005 -62
Branches 852 841 -11
==========================================
- Hits 5192 5124 -68
- Misses 542 547 +5
- Partials 333 334 +1
|
35e889c to
ea16ab0
Compare
There was a problem hiding this comment.
By adding a min delay to LONG delays, we can make sure these requests happen AFTER more critical requests.
There was a problem hiding this comment.
Had to increase the timeout of these a ton with this change, since we need to wait for the product entitlement mapping to finish in many of these tests, which can be delayed up to 10 seconds...
There was a problem hiding this comment.
In iOS I solved that by explicitly requesting the mapping before every test, to force they're available:
https://github.com/RevenueCat/purchases-ios/blob/main/Tests/BackendIntegrationTests/OfflineStoreKitIntegrationTests.swift#L271
I think artificially adding a 5 second delay to every single test is a smell. It's normal if these tests aren't very fast, but why make each of them take 5 extra seconds, that will add up really quickly.
If the iOS approach doesn't work here, we could also mock Delays for these tests.
There was a problem hiding this comment.
Right, in Android, we are also explicitly asking for the mapping too , but the delay is still added (note that that's not a public API, so users won't hold waiting for that data).
We could try to mock Delay, but I'm concerned that if we do that, the integration tests won't reflect an actual production behavior... I guess we could decrease the jittering a lot though, to maybe a couple seconds tops, so we don't have to wait so long for these tests. Will try to do that.
There was a problem hiding this comment.
Mocking wasn't really easy since that class is very internal and it would have increased the complexity of the integration tests a ton. I decided to just separate the jittering values for the integration test flavor that already exists: c8204da.
I also considered creating a flag when running integration tests but those mostly require using reflection or creating build types for tests and would make it so the test code is also present in production code. Also, I think it's not worth it as long as it's just modifying some constants. If we end up having to modify the logic in multiple places for integration tests (we hopefully shouldn't), then I think we can consider adding the flag instead.
There was a problem hiding this comment.
Note that currently only diagnostics and the product entitlement mapping requests have a long delay.
There was a problem hiding this comment.
This is cool, we should do it in iOS.
There was a problem hiding this comment.
In iOS I solved that by explicitly requesting the mapping before every test, to force they're available:
https://github.com/RevenueCat/purchases-ios/blob/main/Tests/BackendIntegrationTests/OfflineStoreKitIntegrationTests.swift#L271
I think artificially adding a 5 second delay to every single test is a smell. It's normal if these tests aren't very fast, but why make each of them take 5 extra seconds, that will add up really quickly.
If the iOS approach doesn't work here, we could also mock Delays for these tests.
There was a problem hiding this comment.
Note the 2 files that are practically identical. For people not familiar with Android, you can separate code by flavor/build type according to the folder where they live. In this case, I create the same file in the defaults and integrationTest flavors with different values.
There was a problem hiding this comment.
This makes a lot of sense 👏🏻
There was a problem hiding this comment.
This makes a lot of sense 👏🏻
…lement mapping have long jitter
7463c32 to
69e4fe1
Compare
**This is an automatic release.** ### New Features * Offline entitlements support (#1030) via Toni Rico (@tonidero) ### Bugfixes * Fix billing connection error when querying purchases early in the process lifetime (#1032) via Toni Rico (@tonidero) ### Performance Improvements * Perform product entitlement mapping request after more critical requests (#1017) via Toni Rico (@tonidero) ### Dependency Updates * Bump fastlane from 2.212.2 to 2.213.0 (#1024) via dependabot[bot] (@dependabot[bot]) ### Other Changes * Get offerings response from disk cache if available (#1029) via Toni Rico (@tonidero) * Improve offline entitlements logs to mention limitations of this mode (#1039) via Toni Rico (@tonidero) * Improve error message when backend returns internal error code (#1038) via Toni Rico (@tonidero) * PurchaseTester: Add new UI to configure internal proxy behavior (#1016) via Toni Rico (@tonidero) * Updated readme to include links to migration guides (#1021) via Marcos Castany (@mcastany) * Store offerings response in SharedPreferences (#1028) via Toni Rico (@tonidero) * Refactor offerings code out of Purchases (#1027) via Toni Rico (@tonidero) --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com>
See also RevenueCat/purchases-android#1017. This will be used for dispatching paywall events
Description
The product entitlement mapping info is not critical most times, so we can perform it as the last step when initializing the SDK as a small optimization.