[EXTERNAL] Add `awaitCustomerInfo` / coroutines tests to `TrustedEntitlementsInformationalModeIntegrationTest` by pablo-guardiola · Pull Request #1077 · RevenueCat/purchases-android

pablo-guardiola · 2023-06-20T21:22:02Z

Checklist

If applicable, unit tests
If applicable, create follow-up issues for purchases-ios and hybrids

Motivation

Why is this change required? What problem does it solve?

Follow up from #1071 (comment)

Description

Describe your changes in detail

Adds awaitCustomerInfo / coroutines tests to TrustedEntitlementsInformationalModeIntegrationTest

Please describe in detail how you tested your changes

Run integration tests locally ✅

cc @tonidero @vegaro @NachoSoto

NachoSoto · 2023-06-20T21:43:21Z

Is there any benefit in keeping both styles of tests?

pablo-guardiola · 2023-06-21T02:05:49Z

Is there any benefit in keeping both styles of tests?

Not really (awaitCustomerInfo is calling getCustomerInfoWith). I added them to showcase how they compare to each other but agreed, having both wouldn't add any value. I'm fine removing the "old" style tests and re-pushing. Please let me know and will accommodate. Thanks for the feedback!

tonidero

Yeah I think we probably should remove the existing tests with the addition of the coroutines versions. Do you mind doing that @pablo-guardiola ? Also left a couple comments.

tonidero · 2023-06-21T08:56:44Z

Well, we shouldn't be using the integrationTest flavor for anything public, but still I think we should add this dependency in androidTestIntegrationTestImplementation to make sure it's only used for instrumentation tests. Thoughts?

I was debating myself between integrationTestImplementation and androidTestIntegrationTestImplementation so sounds good to me 👍

BTW could you clarify what's the reasoning behind 👇?

we shouldn't be using the integrationTest flavor for anything public

Not sure if I fully understand as ultimately whatever dependency you add as integrationTestImplementation or androidTestIntegrationTestImplementation will be only available from the integration tests, right? What am I missing? 🤔

Well, basically I would like to keep the integrationTest flavor as small as possible. If there is any code needed, we should try to add it in the androidTest/androidTestIntegrationTest folder. This is in case we ever remove that flavor in the future.

we shouldn't be using the integrationTest flavor for anything public

That was bad wording on my side 😅 . I mean't that nothing to be available in the integrationTest flavor if possible. We don't deploy the integrationTest flavor, so nothing there will eventually be public. My bad.

would like to keep the integrationTest flavor as small as possible. [...] This is in case we ever remove that flavor in the future.

Makes sense. Thanks for clarifying 👍

We don't deploy the integrationTest flavor, so nothing there will eventually be public.

I don't expect nothing under any test module / flavor to be public 😅

tonidero · 2023-06-21T08:59:09Z

We are currently already waiting for the activity to be ready in the @Before, where we wait with a CountDownLatch. Do we need to also wait here? I guess it's to access the activity and its scope... I was thinking we could cache the activity and clear it in the @After. Wdyt? Might be prone to leaks, but as long as it's cleared it might be ok? Also, we could move that logic to the BasePurchasesIntegrationTest class in order to share it with other integration tests.

Nah, initially I run into UninitializedPropertyAccessExceptions when accessing Purchases.sharedInstance and adding onActivityReady fixed them. With current implementation, onActivityReady is not necessary anymore. I've just tested and all tests are passing ✅

I think we should be good just removing it, good point!

Rebasing from main I've just hit the above-mentioned UninitializedPropertyAccessException 👀

kotlin.UninitializedPropertyAccessException: There is no singleton instance. Make sure you configure Purchases before trying to get the default instance. More info here: https://errors.rev.cat/configuring-sdk at com.revenuecat.purchases.Purchases$Companion.getSharedInstance(Purchases.kt:1558) at com.revenuecat.purchases.trustedentitlements.TrustedEntitlementsInformationalModeIntegrationTest$initialCustomerInfoIsVerified$1.invokeSuspend(TrustedEntitlementsInformationalModeIntegrationTest.kt:38) at com.revenuecat.purchases.trustedentitlements.TrustedEntitlementsInformationalModeIntegrationTest$initialCustomerInfoIsVerified$1.invoke(Unknown Source:8) at com.revenuecat.purchases.trustedentitlements.TrustedEntitlementsInformationalModeIntegrationTest$initialCustomerInfoIsVerified$1.invoke(Unknown Source:4) at com.revenuecat.purchases.BasePurchasesIntegrationTest$runTestActivityLifecycleScope$1.invokeSuspend(BasePurchasesIntegrationTest.kt:176) at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33) at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106) at android.os.Handler.handleCallback(Handler.java:938) at android.os.Handler.dispatchMessage(Handler.java:99) at android.os.Looper.loopOnce(Looper.java:201) at android.os.Looper.loop(Looper.java:288) at android.app.ActivityThread.main(ActivityThread.java:7842) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003) Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@dbe9e1b, Dispatchers.Main.immediate]

I'm investigating 🕵️

It only happens the very first time you launch the tests after a fresh build. I can consistently reproduce doing a Build > Clean Project and launching the tests. It seems something related to how Purchases is initialized / configured on the first run (as it's a singleton). Continue investigating...

Arghh! Can't reproduce anymore 🤔

It seems there's a timing issue hidden somewhere that makes the tests flaky when running locally. Any ideas?

Wondering also if this can be reproduced from main. Will try.

Wondering also if this can be reproduced from main. Will try.

No luck so probably something brought by the Coroutines async nature. Will keep digging.

tonidero · 2023-06-21T09:03:28Z

I'm wondering if we could use runTest for instrumentation tests as well, instead of using the activity scope... I'm not sure of the pros and cons so this might be ok.

I'd rather not. runTest is a coroutine builder designed for unit testing (it behaves similarly to runBlocking, with the difference that the code that it runs will skip delays) but these are integration tests and hence I believe we should reproduce the "production" behavior as much as possible.

Makes sense, thanks!

pablo-guardiola · 2023-06-21T14:08:45Z

@tonidero I've just addressed the feedback. This is ready for another round of 👀

NachoSoto

This is really nice 👍🏻

NachoSoto · 2023-06-21T18:47:20Z

We don't need these "awaits" in the names, that's an implementation detail of how the tests are written now

Suggested change

fun initialCustomerInfoIsVerifiedAwaitCustomerInfo() {

fun initialCustomerInfoIsVerified() {

Great catch! I've removed AwaitCustomerInfo from the names when removing the "old" style tests but missed this one 👍

Fixed and pushed.

NachoSoto · 2023-06-21T18:47:34Z

Is there a way to avoid repeating this in every test?

Good question. Let me think about it and will report back here.

Abstracted it a little by extracting it into 👇

https://github.com/RevenueCat/purchases-android/blob/8d52048c3e34efc951fd2bc862f486af7028389b/purchases/src/androidTestIntegrationTest/kotlin/com/revenuecat/purchases/BasePurchasesIntegrationTest.kt#L172-L178

Please let me know what you think.

tonidero

There is one more thing I think we should fix but I really love how it shortens these tests! Really excited about moving all our integration tests!

tonidero · 2023-06-22T09:35:10Z

Hmm this is not using the FETCH_CURRENT cache fetch policy. Looks like we didn't add that parameter to the coroutines version. I would think this test would fail, since it should be getting the customer info from cache, which is actually verified.

I think we can add that missing parameter in a separate PR and then rebase this off that.

Ohh, good point! I overlooked that one, thanks for the heads up. Glad that we caught something missing from awaitCustomerInfo API that we can improve 🚀

Will cut a separate PR and update this one afterwards 👍

Will cut a separate PR

There you go #1086

update this one afterwards

I've just rebased from main and added CacheFetchPolicy.FETCH_CURRENT to verificationChangesAfterSuccessIsNotified. Wondering though why the test was passing without it 🤔

@vegaro

… API (#1086)  ### Checklist - [ ] If applicable, unit tests - [ ] If applicable, create follow-up issues for `purchases-ios` and hybrids ### Motivation **Why is this change required? What problem does it solve?** Follow up from #1077 (comment)  ### Description **Describe your changes in detail** - Add missing `fetchPolicy` parameter to `awaitCustomerInfo` API **Please describe in detail how you tested your changes** - Run `purchases` tests - Run `examples.purchase-test` app cc @vegaro @tonidero

@pablo-guardiola

… API (#1086) (#1090)  ### Checklist - [ ] If applicable, unit tests - [ ] If applicable, create follow-up issues for `purchases-ios` and hybrids ### Motivation **Why is this change required? What problem does it solve?** Follow up from #1077 (comment)  ### Description **Describe your changes in detail** - Add missing `fetchPolicy` parameter to `awaitCustomerInfo` API **Please describe in detail how you tested your changes** - Run `purchases` tests - Run `examples.purchase-test` app Contributed by @pablo-guardiola Co-authored-by: pablo-guardiola <131195486+pablo-guardiola@users.noreply.github.com>

…ationalModeIntegrationTest

tonidero

Love this! Thanks for handling it! I will get this merged

@pablo-guardiola

…tlementsInformationalModeIntegrationTest` (#1077) via @pablo-guardiola (#1107)  ### Checklist - [ ] If applicable, unit tests - [ ] If applicable, create follow-up issues for `purchases-ios` and hybrids ### Motivation **Why is this change required? What problem does it solve?** Follow up from #1071 (comment)  ### Description **Describe your changes in detail** - Adds `awaitCustomerInfo` / coroutines tests to `TrustedEntitlementsInformationalModeIntegrationTest` **Please describe in detail how you tested your changes** - Run integration tests locally ✅ Contributed by @pablo-guardiola Co-authored-by: pablo-guardiola <131195486+pablo-guardiola@users.noreply.github.com>

…tinuing (#1120) ### Description Looks like our new coroutine test infrastructure added in #1077 / #1107 wasn't waiting for the code to be executed before continuing. In some cases the `@After` code was actually executed before the test, in those cases, the test was failing. In this PR I change it to a blocking method so the test finishes before continuing.

@NachoSoto

**This is an automatic release.** ### New Features * `Trusted Entitlements`: made API stable (#1105) via NachoSoto (@NachoSoto) This new feature prevents MitM attacks between the SDK and the RevenueCat server. With verification enabled, the SDK ensures that the response created by the server was not modified by a third-party, and the entitlements received are exactly what was sent. This is 100% opt-in. `EntitlementInfos` have a new `VerificationResult` property, which will indicate the validity of the responses when this feature is enabled. ```kotlin fun configureRevenueCat() { val configuration = PurchasesConfiguration.Builder(context, apiKey) .entitlementVerificationMode(EntitlementVerificationMode.INFORMATIONAL) .build() Purchases.configure(configuration) } ``` ### Experimental features * Add await offerings (#1096) via Cesar de la Vega (@vegaro) ### Bugfixes * Fix issue updating customer info on app open (#1128) via Toni Rico (@tonidero) ### Dependency Updates * Bump fastlane-plugin-revenuecat_internal from `13773d2` to `b2108fb` (#1095) via dependabot[bot] (@dependabot[bot]) ### Other Changes * [PurchaseTester] Add option to purchase an arbitrary product id (#1099) via Mark Villacampa (@MarkVillacampa) * Fix release path after module refactor (#1129) via Toni Rico (@tonidero) * Fix load shedder integration tests (#1125) via Toni Rico (@tonidero) * Trusted entitlements: New trusted entitlements signature format (#1117) via Toni Rico (@tonidero) * Fix integration tests and change to a different project (#1123) via Toni Rico (@tonidero) * Move files into src/main/kotlin (#1122) via Cesar de la Vega (@vegaro) * Remove public module (#1113) via Cesar de la Vega (@vegaro) * Remove common module (#1106) via Cesar de la Vega (@vegaro) * Fix flaky integration tests: Wait for coroutines to finish before continuing (#1120) via Toni Rico (@tonidero) * Move amazon module into purchases (#1112) via Cesar de la Vega (@vegaro) * Trusted entitlements: Add IntermediateSignatureHelper to handle intermediate signature verification process (#1110) via Toni Rico (@tonidero) * Trusted entitlements: Add Signature type to process new signature response format (#1109) via Toni Rico (@tonidero) * [EXTERNAL] Add `awaitCustomerInfo` / coroutines tests to `TrustedEntitlementsInformationalModeIntegrationTest` (#1077) via @pablo-guardiola (#1107) via Toni Rico (@tonidero) * Remove feature:google module (#1104) via Cesar de la Vega (@vegaro) * Remove identity module (#1103) via Cesar de la Vega (@vegaro) * Remove subscriber attributes module (#1102) via Cesar de la Vega (@vegaro) * Delete utils module (#1098) via Cesar de la Vega (@vegaro) * Remove strings module (#1097) via Cesar de la Vega (@vegaro) * Update CHANGELOG.md to include external contribution (#1100) via Cesar de la Vega (@vegaro) * [EXTERNAL] Add missing `fetchPolicy` parameter to `awaitCustomerInfo` API (#1086) via @pablo-guardiola (#1090) via Toni Rico (@tonidero) --------- Co-authored-by: revenuecat-ops <ops@revenuecat.com> Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com>

pablo-guardiola mentioned this pull request Jun 20, 2023

Trusted entitlements: Add integration tests #1071

Merged

pablo-guardiola force-pushed the pg-trusted-entitlements-integration-tests-coroutines branch from 243c129 to a848d36 Compare June 20, 2023 21:29

tonidero reviewed Jun 21, 2023

View reviewed changes

pablo-guardiola force-pushed the pg-trusted-entitlements-integration-tests-coroutines branch from a848d36 to 0a0c296 Compare June 21, 2023 14:07

NachoSoto reviewed Jun 21, 2023

View reviewed changes

pablo-guardiola force-pushed the pg-trusted-entitlements-integration-tests-coroutines branch from 0a0c296 to 8d52048 Compare June 21, 2023 19:41

tonidero reviewed Jun 22, 2023

View reviewed changes

pablo-guardiola force-pushed the pg-trusted-entitlements-integration-tests-coroutines branch from 8d52048 to b82c9ec Compare June 22, 2023 14:01

pablo-guardiola mentioned this pull request Jun 22, 2023

[EXTERNAL] Add missing fetchPolicy parameter to awaitCustomerInfo API #1086

Merged

2 tasks

tonidero mentioned this pull request Jun 26, 2023

[EXTERNAL] Add missing fetchPolicy parameter to awaitCustomerInfo API (#1086) via @pablo-guardiola #1090

Merged

2 tasks

add awaitCustomerInfo / coroutines tests to TrustedEntitlementsInform…

6ff7da8

…ationalModeIntegrationTest

pablo-guardiola force-pushed the pg-trusted-entitlements-integration-tests-coroutines branch from b82c9ec to 6ff7da8 Compare June 27, 2023 12:44

tonidero approved these changes Jun 28, 2023

View reviewed changes

tonidero changed the title ~~Add awaitCustomerInfo / coroutines tests to TrustedEntitlementsInformationalModeIntegrationTest~~ [EXTERNAL] Add awaitCustomerInfo / coroutines tests to TrustedEntitlementsInformationalModeIntegrationTest Jun 28, 2023

tonidero changed the base branch from main to external/pablo-guardiola/pg-trusted-entitlements-integration-tests-coroutines June 28, 2023 08:30

tonidero merged commit d55a980 into RevenueCat:external/pablo-guardiola/pg-trusted-entitlements-integration-tests-coroutines Jun 28, 2023

pablo-guardiola deleted the pg-trusted-entitlements-integration-tests-coroutines branch June 28, 2023 08:53

This was referenced Jul 3, 2023

Clear coroutines scope after each integration test to fix flakyness #1116

Closed

Fix flaky integration tests: Wait for coroutines to finish before continuing #1120

Merged

RCGitBot mentioned this pull request Jul 10, 2023

[AUTOMATIC] Release/6.6.0 #1130

Merged

	fun initialCustomerInfoIsVerifiedAwaitCustomerInfo() {
	fun initialCustomerInfoIsVerified() {

Uh oh!

Conversation

pablo-guardiola commented Jun 20, 2023

Checklist

Motivation

Description

Uh oh!

NachoSoto commented Jun 20, 2023

Uh oh!

pablo-guardiola commented Jun 21, 2023

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!

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!

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!

pablo-guardiola commented Jun 21, 2023

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!

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 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants