-
Notifications
You must be signed in to change notification settings - Fork 50
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
[EXTERNAL] Add awaitCustomerInfo
/ coroutines tests to TrustedEntitlementsInformationalModeIntegrationTest
#1077
[EXTERNAL] Add awaitCustomerInfo
/ coroutines tests to TrustedEntitlementsInformationalModeIntegrationTest
#1077
Conversation
243c129
to
a848d36
Compare
Is there any benefit in keeping both styles of tests? |
Not really ( |
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.
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.
purchases/build.gradle
Outdated
@@ -69,6 +69,7 @@ dependencies { | |||
integrationTestImplementation 'androidx.appcompat:appcompat:1.4.1' | |||
integrationTestImplementation 'com.google.android.material:material:1.6.0' | |||
integrationTestImplementation 'androidx.constraintlayout:constraintlayout:2.1.3' | |||
integrationTestImplementation("androidx.lifecycle:lifecycle-runtime-ktx:$lifecycleVersion") |
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.
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?
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 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? 🤔
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.
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.
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.
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 😅
@@ -51,6 +54,17 @@ class TrustedEntitlementsInformationalModeIntegrationTest : BasePurchasesIntegra | |||
assertThat(receivedCustomerInfo?.entitlements?.verification).isEqualTo(VerificationResult.VERIFIED) | |||
} | |||
|
|||
@Test | |||
fun initialCustomerInfoIsVerifiedAwaitCustomerInfo() { | |||
onActivityReady { |
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 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.
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.
Nah, initially I run into UninitializedPropertyAccessException
s 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!
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.
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 🕵️
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.
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...
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.
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.
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.
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.
@Test | ||
fun initialCustomerInfoIsVerifiedAwaitCustomerInfo() { | ||
onActivityReady { | ||
activity.lifecycleScope.launch { |
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'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.
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'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.
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.
Makes sense, thanks!
a848d36
to
0a0c296
Compare
@tonidero I've just addressed the feedback. This is ready for another round of 👀 |
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.
}, | ||
) | ||
} | ||
fun initialCustomerInfoIsVerifiedAwaitCustomerInfo() { |
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 don't need these "awaits" in the names, that's an implementation detail of how the tests are written now
fun initialCustomerInfoIsVerifiedAwaitCustomerInfo() { | |
fun initialCustomerInfoIsVerified() { |
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.
Great catch! I've removed AwaitCustomerInfo
from the names when removing the "old" style tests but missed this one 👍
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.
Fixed and pushed.
) | ||
} | ||
fun initialCustomerInfoIsVerifiedAwaitCustomerInfo() { | ||
activity.lifecycleScope.launch { |
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.
Is there a way to avoid repeating this in every test?
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.
Good question. Let me think about it and will report back here.
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.
Abstracted it a little by extracting it into 👇
Lines 172 to 178 in 8d52048
protected fun runTestActivityLifecycleScope( | |
testBody: suspend CoroutineScope.() -> Unit, | |
) { | |
activity.lifecycleScope.launch { | |
testBody() | |
} | |
} |
Please let me know what you think.
0a0c296
to
8d52048
Compare
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 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!
}, | ||
) | ||
} | ||
val receivedCustomerInfo2 = Purchases.sharedInstance.awaitCustomerInfo() |
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.
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.
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.
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 👍
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.
Will cut a separate PR
There you go #1086
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.
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 🤔
8d52048
to
b82c9ec
Compare
… API (#1086) <!-- Thank you for contributing to Purchases! Before pressing the "Create Pull Request" button, please provide the following: --> ### 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) <!-- Please link to issues following this format: Resolves #999999 --> ### 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
… API (#1086) <!-- Thank you for contributing to Purchases! Before pressing the "Create Pull Request" button, please provide the following: --> ### 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) <!-- Please link to issues following this format: Resolves #999999 --> ### 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
… API (#1086) (#1090) <!-- Thank you for contributing to Purchases! Before pressing the "Create Pull Request" button, please provide the following: --> ### 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) <!-- Please link to issues following this format: Resolves #999999 --> ### 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 <[email protected]>
…ationalModeIntegrationTest
b82c9ec
to
6ff7da8
Compare
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.
Love this! Thanks for handling it! I will get this merged
awaitCustomerInfo
/ coroutines tests to TrustedEntitlementsInformationalModeIntegrationTest
awaitCustomerInfo
/ coroutines tests to TrustedEntitlementsInformationalModeIntegrationTest
d55a980
into
RevenueCat:external/pablo-guardiola/pg-trusted-entitlements-integration-tests-coroutines
…tlementsInformationalModeIntegrationTest` (#1077) via @pablo-guardiola (#1107) <!-- Thank you for contributing to Purchases! Before pressing the "Create Pull Request" button, please provide the following: --> ### 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) <!-- Please link to issues following this format: Resolves #999999 --> ### 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 <[email protected]>
…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.
**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 <[email protected]> Co-authored-by: Toni Rico <[email protected]>
Checklist
purchases-ios
and hybridsMotivation
Why is this change required? What problem does it solve?
Follow up from #1071 (comment)
Description
Describe your changes in detail
awaitCustomerInfo
/ coroutines tests toTrustedEntitlementsInformationalModeIntegrationTest
Please describe in detail how you tested your changes
cc @tonidero @vegaro @NachoSoto