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

Integration Tests: improve flaky tests #3163

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Sep 7, 2023

We get a lot of flaky failures on OfflineStoreKitIntegrationTests (example: https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/14843/workflows/a3cffa7c-da25-4811-931d-fc241befee0f/jobs/118214/tests).
I managed to reproduce locally running some of these 100+ times on a loop until they failed.

I traced the issue to this sequence:

  • SK2 purchase
  • StoreKit.Transaction.unfinished is empty

This is likely due to a race condition. In practice, when computing offline CustomerInfo, we never check it right away, since we need to wait for the server to return a 500.

I filed a Radar for this: FB13133387.

To make tests more realistic, I introduced a delay in HTTPClient returning fake 5xx errors. I haven't been able to reproduce this issue again, so this should make OfflineStoreKitIntegrationTests more reliable.

I've also added a new debug log that could help debug similar issues, because Strings.offline.computed_offline_customer_info was not very complete.

We get a lot of flaky failures on `OfflineStoreKitIntegrationTests` (example: https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/14843/workflows/a3cffa7c-da25-4811-931d-fc241befee0f/jobs/118214/tests).
I managed to reproduce locally running some of these 100+ times on a loop until they failed.

I traced the issue to this sequence:
- SK2 purchase
- `StoreKit.Transaction.unfinished` is empty

This is likely due to a race condition. In practice, when computing offline `CustomerInfo`, we never check it right away, since we need to wait for the server to return a 500.

To make tests more realistic, I introduced a delay in `HTTPClient` returning fake 5xx errors. I haven't been able to reproduce this issue again, so this should make `OfflineStoreKitIntegrationTests` more reliable.

I've also added a new debug log that could help debug similar issues, because `Strings.offline.computed_offline_customer_info` was not very complete.
@NachoSoto NachoSoto added the test label Sep 7, 2023
@NachoSoto NachoSoto requested a review from a team September 7, 2023 16:54
@@ -113,7 +113,7 @@ extension NetworkStrings: LogMessage {

#if DEBUG
case let .api_request_forcing_server_error(request):
return "Returning fake HTTP 500 error for '\(request.description)'"
return "Returning fake HTTP 500 error for \(request.description)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ended up with double quotes:

Returning fake HTTP 500 error for 'POST '/v1/receipts''

Logger.info(Strings.offlineEntitlements.computed_offline_customer_info(
products, offlineCustomerInfo.entitlements
))
Logger.debug(Strings.offlineEntitlements.computed_offline_customer_info_details(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I improved these logs to be able to debug future issues here. Initially I wasn't sure if PurchasedProductsFetcher was working correctly.

@aboedo
Copy link
Member

aboedo commented Sep 7, 2023

I traced the issue to this sequence:

SK2 purchase
StoreKit.Transaction.unfinished is empty

Do we have a radar for this? It doesn't seem like expected behavior. Just to clarify, we check unfinished after we get the response from await purchase, right?

completionHandler?(
.failure(.errorResponse(Self.serverErrorResponse, .internalServerError))
)
DispatchQueue.main.asyncAfter(deadline: .now() + .milliseconds(300)) {
Copy link
Member

Choose a reason for hiding this comment

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

the intent behind this isn't obvious at first glance so let's make sure to add a comment here

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 point, I always think it's in git history but that's easily lost.

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.

@NachoSoto
Copy link
Contributor Author

Just to clarify, we check unfinished after we get the response from await purchase, right?

Yup

Do we have a radar for this? It doesn't seem like expected behavior.

Nope. I'll file one 👍🏻

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (5527f88) 86.61% compared to head (f9bdb31) 86.63%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3163      +/-   ##
==========================================
+ Coverage   86.61%   86.63%   +0.01%     
==========================================
  Files         220      220              
  Lines       15715    15738      +23     
==========================================
+ Hits        13611    13634      +23     
  Misses       2104     2104              
Files Changed Coverage Δ
Sources/Logging/Strings/NetworkStrings.swift 100.00% <100.00%> (ø)
...s/Logging/Strings/OfflineEntitlementsStrings.swift 87.14% <100.00%> (+3.50%) ⬆️
Sources/Networking/HTTPClient/HTTPClient.swift 97.88% <100.00%> (+0.01%) ⬆️
...flineEntitlements/OfflineCustomerInfoCreator.swift 91.93% <100.00%> (+0.70%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NachoSoto
Copy link
Contributor Author

Filed: FB13133387

@NachoSoto NachoSoto enabled auto-merge (squash) September 7, 2023 17:27
@NachoSoto NachoSoto disabled auto-merge September 7, 2023 17:27
@NachoSoto NachoSoto enabled auto-merge (squash) September 7, 2023 17:27
@NachoSoto NachoSoto merged commit 341f0d3 into main Sep 7, 2023
@NachoSoto NachoSoto deleted the offline-integration-tests-flaky branch September 7, 2023 19:25
NachoSoto pushed a commit that referenced this pull request Sep 14, 2023
**This is an automatic release.**

### Bugfixes
* Fix runtime crash in SK2TransactionListener in iOS < 15 (#3206) via
Toni Rico (@tonidero)
### Performance Improvements
* `OperationDispatcher`: add support for "long" delays (#3168) via
NachoSoto (@NachoSoto)
### Other Changes
* `Integration Tests`: add tests for ghost transfer behavior (#3135) via
NachoSoto (@NachoSoto)
* `Xcode`: removed `purchases-ios` SPM reference (#3166) via NachoSoto
(@NachoSoto)
* `Integration Tests`: another flaky failure (#3165) via NachoSoto
(@NachoSoto)
* `Integration Tests`: fix flaky test failure due to leftover
transaction (#3167) via NachoSoto (@NachoSoto)
* `Xcode 13`: removed last `Swift 5.7` checks (#3161) via NachoSoto
(@NachoSoto)
* `Integration Tests`: improve flaky tests (#3163) via NachoSoto
(@NachoSoto)
* `Codable`: improved decoding errors (#3153) via NachoSoto (@NachoSoto)
* Refactor: extract `HealthOperation` (#3154) via NachoSoto (@NachoSoto)
* `Xcode 13`: remove conditional code (#3147) via NachoSoto (@NachoSoto)
* `CircleCI`: change all jobs to use `Xcode 14.x` and replace
`xcode-install` with `xcodes` (#2421) via NachoSoto (@NachoSoto)
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.

2 participants