Performance: throttle cache updates, and avoid double updates on app launch#2983
Performance: throttle cache updates, and avoid double updates on app launch#2983NachoSoto wants to merge 1 commit into
Performance: throttle cache updates, and avoid double updates on app launch#2983Conversation
21c7ab1 to
0bab77e
Compare
There was a problem hiding this comment.
This was only 2 because fireNotifications was firing the didEnterForeground and didEnterBackground, and this test was detecting the double update.
Now it's 1 because it only gets updated automatically (regardless of throttling) when entering the background.
Codecov Report
@@ Coverage Diff @@
## main #2983 +/- ##
==========================================
+ Coverage 86.68% 86.73% +0.05%
==========================================
Files 219 218 -1
Lines 15636 15636
==========================================
+ Hits 13554 13562 +8
+ Misses 2082 2074 -8
|
joshdholtz
left a comment
There was a problem hiding this comment.
This looks good to me! Just a little thought on naming 🤷♂️
There was a problem hiding this comment.
I think this naming might be a little confusing here 🤔 setLastCacheUpdateTime() makes it seem like this method should be called inside of updateAllCachesIfNeeded() when its completed. Should it some like... setLastCacheFetchedTime() 🤷♂️
tonidero
left a comment
There was a problem hiding this comment.
A couple comments but nothing blocking
There was a problem hiding this comment.
This is now throttled in the applicationWillEnterForeground but not on the applicationDidEnterBackground... Not a blocker but wanted to mention it.
There was a problem hiding this comment.
I feel we should extract all the code here to an updateCachesOnAppForeground or something like that, and move this there... Just in case we want to do something else on app foreground and we don't want to skip it. NABD though.
…p launch Possibly as a consequence of #2623 we were calling `updateAllCachesIfNeeded` on app launch together with `updateCachesIfInForeground`, meaning that we were updating all caches twice. Thanks to our de-duping logic this is normally not a big issue, but this issue combined with what #2954 fixes, means that if there are pending transactions, opening the app would probably end up posting receipts duplicated times. This adds a new log as well to detect this: > [purchases] DEBUG: ℹ️ Throttling cache update, only 0.8 seconds elapsed And a new integration test that posts `UIApplication.willEnterForegroundNotification` to test this.
0bab77e to
cd6213e
Compare
|
Decided that we don't need this. |
…quest de-dupping This adds more coverage for why we don't actually need #2983.
This extracts a useful refactor from #2983: `ClockType` now lives in `SystemInfo`, so types that depend on type can use the same clock, and tests can rely on the mocked `TestClock`.
…undNotification` to simulate a real app #2983 was an attempt to work around this. Instead, this PR adds integration tests to cover this behavior and ensure that we don't make duplicate requests.
Possibly as a consequence of #2623 we were calling
updateAllCachesIfNeededon app launch together withupdateCachesIfInForeground, meaning that we were updating all caches twice.Thanks to our de-duping logic this is normally not a big issue, but this issue combined with what #2954 fixes, means that if there are pending transactions, opening the app would probably end up posting receipts duplicated times.
This adds a new log as well to detect this:
And a new integration test that posts
UIApplication.willEnterForegroundNotificationto test this.