Skip to content
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

Trusted entitlements: Add integration tests #1071

Merged
merged 2 commits into from
Jun 19, 2023

Conversation

tonidero
Copy link
Contributor

Description

This adds integration tests to verify the basic behavior of the trusted entitlements feature.

@tonidero tonidero added the test label Jun 15, 2023
@tonidero tonidero marked this pull request as ready for review June 15, 2023 09:04
@tonidero tonidero requested a review from a team June 15, 2023 09:04
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #1071 (dc6a1fa) into main (d669a99) will increase coverage by 0.00%.
The diff coverage is 90.00%.

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

@@           Coverage Diff           @@
##             main    #1071   +/-   ##
=======================================
  Coverage   85.64%   85.64%           
=======================================
  Files         176      176           
  Lines        6247     6256    +9     
  Branches      859      861    +2     
=======================================
+ Hits         5350     5358    +8     
  Misses        563      563           
- Partials      334      335    +1     
Impacted Files Coverage Δ
.../java/com/revenuecat/purchases/common/AppConfig.kt 81.63% <66.66%> (-0.98%) ⬇️
...at/purchases/common/verification/SigningManager.kt 100.00% <100.00%> (ø)
...otlin/com/revenuecat/purchases/PurchasesFactory.kt 84.21% <100.00%> (+0.24%) ⬆️

Copy link
Contributor

@NachoSoto NachoSoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start 👏🏻 just a question about test setup to be able to test changing conditions (I still have to add tests like that in iOS too).

Comment on lines 42 to 45
if (appConfig.forceSigningErrors) {
warnLog("Forcing signing error for request with path: $urlPath")
return VerificationResult.FAILED
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish there was a way to ensure this cannot run in production builds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah honestly it's not great... With the server version of this I was thinking of a way by creating a new class that exists in each flavor (the integration test and the defaults flavors). The defaults flavor version wouldn't do anything and the integration test flavor would indicate the failure, however, we would still have to use that code here and it would complicate things more... I would really like to have what we have in iOS where we can make code compile only with certain flags but that's more difficult to architecture in Android and not sure if it's worth it...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually having a file that's different on each flavor, wouldn't it be possible to at least detect the flavor at runtime?
So AppConfig for example can't even allow this value being true in the "release" flavor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! We actually can get the flavor from BuildConfig.FLAVOR without extra files. We can use that to do what you said yeah. It would still have this code in the final bundle, but it would be another safety measure, I can do it like that 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you're thinking of something better, but we could change AppConfig so that some values can never return anything but the default depending on BuildConfig.FLAVOR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in the base PR: e16c410

@@ -16,6 +16,7 @@ class AppConfig(
val dangerousSettings: DangerousSettings = DangerousSettings(autoSyncPurchases = true),
// Should only be used for tests
var forceServerErrors: Boolean = false,
var forceSigningErrors: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe to be overly cautious it would help to write a unit test to verify that this is false by default, as a way to ensure production builds never ship with this on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in the base PR in 371d747


@OptIn(ExperimentalPreviewRevenueCatPurchasesAPI::class)
@RunWith(AndroidJUnit4::class)
class TrustedEntitlementsInformationalFailureIntegrationTest : BasePurchasesIntegrationTest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe TrustedEntitlementsInformationalModeIntegrationTest? Since you might want to test successes too?
Oh I see you have a separate one. I'm usually not against splitting test classes, easier to maintain. But I think it might be weird because it would be really useful to add tests that verify the behavior when the signature goes from correct to invalid and vice-versa. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I added one in the success file, but it's true it doesn't fully fit... I was torn because I didn't want to make a huge test class and it's easier to setup the condition in the @Before of the test, but well I think we don't have that many currently and we can indeed modify these things later in the integration test, so I will change it back to only one class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in dc6a1fa

@tonidero tonidero changed the base branch from main to toniricodiez/sdk-3182-add-forceverificationerrors-dangerous-setting-for June 16, 2023 07:05
@tonidero tonidero requested a review from NachoSoto June 16, 2023 07:11
@Test
fun initialCustomerInfoIsVerified() {
var receivedCustomerInfo: CustomerInfo? = null
ensureBlockFinishes { latch ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't wait to see these with coroutines 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty straightforward and neat 💅 #1077

}

@Test
fun canPurchaseProductFailingToVerifyStatus() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice 👍🏻

Base automatically changed from toniricodiez/sdk-3182-add-forceverificationerrors-dangerous-setting-for to main June 19, 2023 06:39
@tonidero tonidero force-pushed the add-trusted-entitlements-integration-tests branch from fdbc826 to 3d0190f Compare June 19, 2023 06:41
@tonidero tonidero enabled auto-merge (squash) June 19, 2023 06:41
@tonidero tonidero merged commit 8ae7c65 into main Jun 19, 2023
@tonidero tonidero deleted the add-trusted-entitlements-integration-tests branch June 19, 2023 06:51
tonidero pushed a commit that referenced this pull request Jun 23, 2023
**This is an automatic release.**

### Bugfixes
* Default customer info schema version to latest known by SDK (#1080)
via Toni Rico (@tonidero)
* Handle other diagnostics-related exceptions (#1076) via Toni Rico
(@tonidero)
* Return error in queryPurchases if error connecting to billing client
(#1072) via Toni Rico (@tonidero)
### Other Changes
* Fix offline entitlements integration tests (#1085) via Toni Rico
(@tonidero)
* Add defaultsRelease variant tests run configuration (#1074) via Toni
Rico (@tonidero)
* Compose sample app: move to gradle catalog (#1081) via Toni Rico
(@tonidero)
* Compose sample app: automate builds (#1082) via Toni Rico (@tonidero)
* Compose sample app (#1056) via Toni Rico (@tonidero)
* Migrate to Gradle version catalog (#1059) via Cesar de la Vega
(@vegaro)
* Trusted entitlements: Add logs with verification mode (#1067) via Toni
Rico (@tonidero)
* Sync pending purchases before getting customer info (#1073) via Toni
Rico (@tonidero)
* Refactor syncing pending transactions logic out of `Purchases` (#1058)
via Toni Rico (@tonidero)
* Refactor CustomerInfo listener and cache logic into
CustomerInfoUpdater (#1052) via Toni Rico (@tonidero)
* Trusted entitlements: Add integration tests (#1071) via Toni Rico
(@tonidero)
* Trusted entitlements: Add internal mechanism to force signing errors
for tests (#1070) via Toni Rico (@tonidero)

Co-authored-by: revenuecat-ops <[email protected]>
tonidero pushed a commit that referenced this pull request Jun 28, 2023
…tlementsInformationalModeIntegrationTest` (#1077)

<!-- 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 ✅ 

cc @tonidero @vegaro @NachoSoto
tonidero added a commit that referenced this pull request Jun 29, 2023
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants