Skip to content

Support forcing server errors for tests#1008

Merged
tonidero merged 3 commits into
mainfrom
support-forcing-server-errors
May 18, 2023
Merged

Support forcing server errors for tests#1008
tonidero merged 3 commits into
mainfrom
support-forcing-server-errors

Conversation

@tonidero

Copy link
Copy Markdown
Contributor

Description

In iOS, this flag lives within DangerousSettings. In Android, that's not possible due to our module setup, so I moved this flag to AppConfig, which is internal, so there won't be a public API.

@tonidero tonidero added the test label May 18, 2023
requestHeaders: Map<String, String>,
refreshETag: Boolean = false
): HTTPResult {
if (appConfig.forceServerErrors) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In iOS, we additionally have a compile time flag so this is only available for debug builds. It's possible to split that up in Android by having different versions in debug/release folders but not sure it's worth it...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe at least make the log in the line below a warning like I did in iOS? It's a pretty important thing if it's happening in a context that we don't expect.

if (callResult == null) {
log(LogIntent.WARNING, NetworkStrings.ETAG_RETRYING_CALL)
return performRequest(baseURL, endpoint, body, requestHeaders, refreshETag = true)
callResult = performRequest(baseURL, endpoint, body, requestHeaders, refreshETag = true)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed this to make the linter happy (Too many returns)

proxyURL: URL?,
overrideBillingAbstract: BillingAbstract? = null
overrideBillingAbstract: BillingAbstract? = null,
forceServerErrors: Boolean = false

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that there are 2 ways of setting the flag, when first initializing the SDK (from tests) or dynamically later by changing the flag in AppConfig. The reason for that is to be able to test situations with a fresh install where all requests fail initially. Otherwise, the initial requests performed during the SDK initialization won't be forced to be errors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That makes sense.

@codecov

codecov Bot commented May 18, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1008 (d4c04e9) into main (78cd322) will increase coverage by 0.01%.
The diff coverage is 94.44%.

❗ Current head d4c04e9 differs from pull request most recent head 5b1e98a. Consider uploading reports for the commit 5b1e98a to get more accurate results

@@            Coverage Diff             @@
##             main    #1008      +/-   ##
==========================================
+ Coverage   85.34%   85.35%   +0.01%     
==========================================
  Files         168      168              
  Lines        5984     5997      +13     
  Branches      835      837       +2     
==========================================
+ Hits         5107     5119      +12     
  Misses        546      546              
- Partials      331      332       +1     
Impacted Files Coverage Δ
.../java/com/revenuecat/purchases/common/AppConfig.kt 82.60% <80.00%> (-1.12%) ⬇️
...java/com/revenuecat/purchases/common/HTTPClient.kt 88.57% <100.00%> (+0.69%) ⬆️
...otlin/com/revenuecat/purchases/PurchasesFactory.kt 82.20% <100.00%> (+0.30%) ⬆️

@tonidero tonidero marked this pull request as ready for review May 18, 2023 12:35
@tonidero tonidero requested a review from a team May 18, 2023 12:35

@NachoSoto NachoSoto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍🏻

}

@Test
fun `can dynamically change between getting server errors and not`() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool

proxyURL: URL?,
overrideBillingAbstract: BillingAbstract? = null
overrideBillingAbstract: BillingAbstract? = null,
forceServerErrors: Boolean = false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That makes sense.

@tonidero tonidero enabled auto-merge (squash) May 18, 2023 16:01
@tonidero tonidero merged commit c924d62 into main May 18, 2023
@tonidero tonidero deleted the support-forcing-server-errors branch May 18, 2023 16:12
This was referenced May 24, 2023
tonidero added a commit that referenced this pull request May 25, 2023
**This is an automatic release.**

### New Features
* Support DEFERRED mode (#985) via swehner (@swehner)
* Add completion callback to syncPurchases API (#1002) via Toni Rico
(@tonidero)
### Bugfixes
* Workaround bug in android 4 for JSON objects with List<String> (#942)
via Andy Boedo (@aboedo)
### Dependency Updates
* Bump fastlane-plugin-revenuecat_internal from `fe45299` to `13773d2`
(#1015) via dependabot[bot] (@dependabot[bot])
### Other Changes
* Bump dokka to 1.8.10 to support Gradle 8 (#1009) via Toni Rico
(@tonidero)
* Disable offline entitlements temporarily (#1023) via Toni Rico
(@tonidero)
* Fix integration tests in CI (#1019) via Toni Rico (@tonidero)
* Add offline entitlements integration tests (#1006) via Toni Rico
(@tonidero)
* Disable offline entitlements in observer mode (#1014) via Toni Rico
(@tonidero)
* Extracts setup and teardown to BasePurchasesTest (#1011) via Cesar de
la Vega (@vegaro)
* Support forcing server errors for tests (#1008) via Toni Rico
(@tonidero)

---------

Co-authored-by: revenuecat-ops <ops@revenuecat.com>
Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com>
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