Add offline entitlements integration tests by tonidero · Pull Request #1006 · RevenueCat/purchases-android

tonidero · 2023-05-16T15:21:24Z

Description

Depends on #987

This PR adds some integration tests checking some basic behaviors of offline entitlements. Mostly making sure the mode is turn on and off as expected.

tonidero · 2023-05-16T15:21:48Z

I moved some logic from PurchasesIntegrationTest to this base class.

tonidero · 2023-05-16T15:23:08Z

This was added because onActivityReady wasn't guaranteed to be executed in order which resulted in some flaky tests. This way, we execute the test setup code and the test itself within a single onActivityReady.

tonidero · 2023-05-16T15:23:43Z

Copied this from the BackendIntegrationTest which are actually unit tests unlike this.

tonidero · 2023-05-16T15:26:03Z

Ideally we should also migrate these tests to use a single onActivityReady. But will do that in a different PR.

tonidero · 2023-05-16T15:27:18Z

It's not great to duplicate these cache keys... But I didn't want to expose them in the common module just for this... I think it's ok? Lmk what you think

tonidero · 2023-05-16T15:33:47Z

Creating these fake products is a bit hacky... We could create them from mocked google product details but that also doesn't feel great to me though, I think this is relatively simpler. Note that this is needed whether we use a proxy or not since we can't get the product info from Google directly.

tonidero · 2023-05-16T15:36:17Z

Still want to write some more tests including:

Does not enter offline entitlements if it's a non-500 error
Recovers from offline entitlements if either a getCustomerInfo or a postReceipt request succeed
...

codecov · 2023-05-16T15:50:41Z

Codecov Report

Merging #1006 (f2b61ce) into main (594138c) will decrease coverage by 0.04%.
The diff coverage is 50.00%.

❗ Current head f2b61ce differs from pull request most recent head f59ea0d. Consider uploading reports for the commit f59ea0d to get more accurate results

@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
- Coverage   85.40%   85.37%   -0.04%     
==========================================
  Files         169      169              
  Lines        5997     6002       +5     
  Branches      836      839       +3     
==========================================
+ Hits         5122     5124       +2     
- Misses        544      545       +1     
- Partials      331      333       +2

Impacted Files	Coverage Δ
.../offlineentitlements/OfflineEntitlementsManager.kt	`89.36% <50.00%> (-3.67%)`	⬇️
.../main/kotlin/com/revenuecat/purchases/Purchases.kt	`82.87% <50.00%> (-0.11%)`	⬇️

tonidero

This is still WIP but I would like some feedback on the approach @RevenueCat/coresdk .

I think this is much simpler than having to run a separate process for the proxy and enable/disable different endpoints in the middle of the test. On the other hand, I agree that hardcoding the success responses makes it much more brittle. I believe that it's somewhat acceptable to hardcode the responses, since we are hitting the backend in other integration tests. We can still use the proxy for manual testing though. Lmk what you think!

tonidero · 2023-05-17T15:17:28Z

Added these to be able to mock the initial state of shared preferences and active purchases within the integration test.

tonidero

These tests don't look nearly as nice in Android as they do in iOS. One of the main differences is coroutine support. That would make the tests much more legible. I tried to avoid as many callbacks as possible but still not nearly as nice. I guess another reason to consider supporting coroutines soon

tonidero · 2023-05-19T14:36:25Z

Added a completion here to be able to wait upon it on tests. It won't be used in production. Still it's tested.

As an improvement you could forward the error too so integration tests can ensure it actually worked?

tonidero · 2023-05-19T14:42:26Z

Had to increase the timeout... Honestly this is already large, but in Android we do all requests sequentially and that's taking a long time for the new tests. Also, randomly the tests will take twice as long... We will probably want to improve the performance of the integration tests soon, but for now I wanted to get them running first. Also, the fact that we are using callbacks and not coroutines makes it necessary for us to put a timeout for the whole test :(

iOS integration tests are also not the fastest because we need to wait on purchase expirations. I think it's okay that these tests aren't the fastest, as long as unit tests are.

tonidero · 2023-05-19T14:58:52Z

This is ready for review @RevenueCat/coresdk

tonidero · 2023-05-19T15:22:24Z

Note that I didn't add automated tests that tested offline entitlements with multiple purchases. Reasons being:

I would need to assume many things about how Google works in order to mock it realistically. I really wish Google released something to test purchases more easily :(.

We would need another valid token for a different product if we wanted to test it against the backend

tonidero · 2023-05-19T15:26:48Z

I wanted to note that this only works for these tests that are using offline entitlements. The token we currently are using for integration tests is for a expired purchase, that means that the backend won't return it as an active purchase. For these tests I'm mocking the purchase as returned from the Google method "queryPurchases" which returns active purchases, that's why it ends up as an active purchase there.

That's also the reason why I'm not verifying that the purchases are active after recovering from offline entitlements mode and only verifying that the purchase is acknowledged and sent to the backend.

This is not really great. I think we can improve this a lot by having an active purchase forever in a company credit card. Will study this option and try to improve this a bit.

NachoSoto

This is a great start 👍🏻

NachoSoto · 2023-05-19T15:27:55Z

As an improvement you could forward the error too so integration tests can ensure it actually worked?

NachoSoto · 2023-05-19T15:29:09Z

iOS integration tests are also not the fastest because we need to wait on purchase expirations. I think it's okay that these tests aren't the fastest, as long as unit tests are.

NachoSoto · 2023-05-19T15:34:16Z

Is it a problem that this is in this order? The code waiting for the latch would be notified before you fail the test.

Right, I think we should completely remove the countdown in these error cases, I added it on an early iteration but it's not really needed, and can cause errors as you said. Good catch!

NachoSoto · 2023-05-19T15:34:48Z

Maybe verify the error is a server error and not something else?

NachoSoto · 2023-05-19T15:34:59Z

Ditto order and in the other tests.

NachoSoto · 2023-05-19T15:36:02Z

This is a noun, so postSetup makes sense 😅

tonidero · 2023-05-22T11:58:17Z

Thanks for the suggestions @NachoSoto! Refactored the integration tests pretty heavily into different classes. So will need another review 🙏

NachoSoto · 2023-05-22T18:47:14Z

…in tests

### Description After adding the integration tests for offline entitlements in #1006, I was testing running each set of tests separately. However, I was changing the parameters we use to run integration tests between normal integration tests and the offline entitlement tests in order to make them pass. In order to make them compatible, I'm just hardcoding the entitlements we verify are granted after purchases during offline entitlements integration tests.

@swehner

**This is an automatic release.** ### New Features * Support DEFERRED mode (#985) via swehner (@swehner) * Add completion callback to syncPurchases API (#1002) via Toni Rico (@tonidero) ### Bugfixes * Workaround bug in android 4 for JSON objects with List<String> (#942) via Andy Boedo (@aboedo) ### Dependency Updates * Bump fastlane-plugin-revenuecat_internal from `fe45299` to `13773d2` (#1015) via dependabot[bot] (@dependabot[bot]) ### Other Changes * Bump dokka to 1.8.10 to support Gradle 8 (#1009) via Toni Rico (@tonidero) * Disable offline entitlements temporarily (#1023) via Toni Rico (@tonidero) * Fix integration tests in CI (#1019) via Toni Rico (@tonidero) * Add offline entitlements integration tests (#1006) via Toni Rico (@tonidero) * Disable offline entitlements in observer mode (#1014) via Toni Rico (@tonidero) * Extracts setup and teardown to BasePurchasesTest (#1011) via Cesar de la Vega (@vegaro) * Support forcing server errors for tests (#1008) via Toni Rico (@tonidero) --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com>

tonidero added the test label May 16, 2023

tonidero commented May 16, 2023

View reviewed changes

tonidero commented May 17, 2023

View reviewed changes

tonidero requested a review from a team May 17, 2023 15:26

tonidero force-pushed the enable-offline-entitlements branch from c6f739b to 8d526d4 Compare May 19, 2023 10:06

tonidero force-pushed the toniricodiez/sdk-3126-offline-entitlements-add-integration branch from ee9953f to fcadf07 Compare May 19, 2023 10:08

tonidero commented May 19, 2023

View reviewed changes

tonidero marked this pull request as ready for review May 19, 2023 14:58

tonidero commented May 19, 2023

View reviewed changes

NachoSoto approved these changes May 19, 2023

View reviewed changes

tonidero requested a review from NachoSoto May 22, 2023 11:58

NachoSoto approved these changes May 22, 2023

View reviewed changes

Base automatically changed from enable-offline-entitlements to main May 23, 2023 08:46

tonidero force-pushed the toniricodiez/sdk-3126-offline-entitlements-add-integration branch from 10ed485 to f2b61ce Compare May 23, 2023 08:48

tonidero enabled auto-merge (squash) May 23, 2023 08:49

tonidero force-pushed the toniricodiez/sdk-3126-offline-entitlements-add-integration branch from 91a1706 to f2b61ce Compare May 23, 2023 09:06

tonidero added 10 commits May 23, 2023 11:08

Add offline entitlements integration tests

4ae07e2

Simplify tests

a2a00f0

Change from using MockWebServer to new flag to force server errors

34f32fe

Adding utilities for integration tests

320a75d

Change to wait for real responses instead of mocking them

6344671

Cleanup tests

dbe782e

Add more tests and some cleanup

19d7c9b

Revert unneeded change after latest changes

439fcb3

Add one more test

36ccafb

Add error to product entitlement mapping update completion to verify …

fa76b57

…in tests

tonidero added 5 commits May 23, 2023 11:10

Some fixes from review 1

ea50055

Refactor tests to split them among multiple subclasses

1008e2e

Remove countdown from error cases to avoid possible issues

777475a

Cleanup and add some verifications

79346fe

Small rename

f59ea0d

tonidero force-pushed the toniricodiez/sdk-3126-offline-entitlements-add-integration branch from f2b61ce to f59ea0d Compare May 23, 2023 09:10

tonidero merged commit 6f48f28 into main May 23, 2023

tonidero deleted the toniricodiez/sdk-3126-offline-entitlements-add-integration branch May 23, 2023 09:26

tonidero mentioned this pull request May 23, 2023

Fix integration tests in CI #1019

Merged

This was referenced May 24, 2023

[AUTOMATIC] Release/6.3.0 #1022

Closed

[AUTOMATIC] Release/6.3.0 #1025

Merged

Uh oh!

Conversation

tonidero commented May 16, 2023

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero commented May 16, 2023

Uh oh!

codecov Bot commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero commented May 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero commented May 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

tonidero May 16, 2023 •

edited

Loading

codecov Bot commented May 16, 2023 •

edited

Loading