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

StoreKitIntegrationTests: removed incorrect waitUntilCustomerInfoIsUpdated and fixed race condition #1492

Merged
merged 5 commits into from
Apr 13, 2022

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Apr 11, 2022

This is probably a leftover from before integration tests got changed to async.

Purchases initialization can fetch an updated CustomerInfo, so assuming this will be 1 is a race condition waiting to happen, because there are potentially multiple customer info requests happening.

Also, SDK initialization begins with an initial request to offerings, which results in a get-create of the initial anonymous user. To avoid race conditions with when this request finishes and make all tests deterministic, integration tests now wait for the initial get offerings to finish

Changes:

  • Removed waitUntilCustomerInfoIsUpdated
  • verifyEntitlementWentThrough now calls verifyEntitlementWentThrough to verify the CustomerInfo has the correct entitlement
  • No longer checking the PurchasesDelegate.customerInfo in all tests, which could lead to race conditions
  • Added a single test that verifies the delegate is notified. This might still be flaky, but at least this issue should be isolated to that new test now.
  • Fixed race condition

@NachoSoto NachoSoto force-pushed the store-kit-integration-tests-fix branch from 199de6c to ee63201 Compare April 11, 2022 23:16
@NachoSoto NachoSoto changed the title StoreKitIntegrationTests: removed incorrect waitUntilCustomerInfoIsUpdated StoreKitIntegrationTests: removed incorrect waitUntilCustomerInfoIsUpdated and improved all assertions Apr 11, 2022
@NachoSoto NachoSoto marked this pull request as ready for review April 11, 2022 23:17
@NachoSoto NachoSoto requested a review from a team April 11, 2022 23:17
@NachoSoto
Copy link
Contributor Author

Still looking into the flaky integration test failures.

@NachoSoto NachoSoto changed the title StoreKitIntegrationTests: removed incorrect waitUntilCustomerInfoIsUpdated and improved all assertions StoreKitIntegrationTests: removed incorrect waitUntilCustomerInfoIsUpdated and fixed race condition Apr 12, 2022
@NachoSoto NachoSoto force-pushed the store-kit-integration-tests-fix branch from fc15fbb to b14803a Compare April 12, 2022 19:40
) async throws -> PurchaseResultData {
let data = try await Purchases.shared.purchase(package: self.monthlyPackage)

try self.verifyEntitlementWentThrough(data.customerInfo,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All purchases through this method now verify the entitlement for good measure (instead of most tests doing that repeatedly).

@NachoSoto NachoSoto force-pushed the store-kit-integration-tests-fix branch 2 times, most recently from caf0340 to 49e9440 Compare April 12, 2022 19:43
@NachoSoto NachoSoto force-pushed the store-kit-integration-tests-fix branch from 49e9440 to bec70ab Compare April 12, 2022 22:16
@NachoSoto
Copy link
Contributor Author

It's interesting, looking at the trend of failures in main it seems like #1484 is what made the failures more frequent.
Either way, I'm glad we managed to find (at least one of) the race condition.

Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

I was convinced I had approved already!! 😅

@NachoSoto NachoSoto merged commit b4f739b into main Apr 13, 2022
@NachoSoto NachoSoto deleted the store-kit-integration-tests-fix branch April 13, 2022 21:27
NachoSoto added a commit that referenced this pull request Mar 24, 2023
Turns out we had already solved this race condition in #1492 that lead to flaky failures,
but it was only in `BaseStoreKitIntegrationTests`. This moves it up to `BaseBackendIntegrationTests` so that
`SubscriberAttributesManagerIntegrationTests` can use it too.

Fixes https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/10501/workflows/cb6dbf22-cda8-4c41-875b-04b489992631/jobs/61135
aboedo pushed a commit that referenced this pull request Mar 27, 2023
…2381)

Turns out we had already solved this race condition in #1492 that lead
to flaky failures,
but it was only in `BaseStoreKitIntegrationTests`. This moves it up to
`BaseBackendIntegrationTests` so that
`SubscriberAttributesManagerIntegrationTests` can use it too.

Fixes
https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/10501/workflows/cb6dbf22-cda8-4c41-875b-04b489992631/jobs/61135
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.

None yet

2 participants