Tests: ensure Purchases is not configured multiple times#2100
Merged
Conversation
e4e5099 to
dc0acb0
Compare
NachoSoto
added a commit
that referenced
this pull request
Nov 30, 2022
Follow up to #2100. This has uncovered a huge source of leaks across many tests. Fixing this will provide many benefits: - Faster test runs - Lower memory overhead - Isolated state across each test run - Ensure that these types don't leak outside of tests too
vegaro
approved these changes
Dec 7, 2022
|
|
||
| #if DEBUG | ||
|
|
||
| private extension UIApplication { |
Member
There was a problem hiding this comment.
Why was this an extension on UIApplication before?
Contributor
Author
There was a problem hiding this comment.
For no particular reason :P
aboedo
reviewed
Dec 7, 2022
Comment on lines
+503
to
+507
| #if DEBUG | ||
| if ProcessInfo.isRunningUnitTests { | ||
| preconditionFailure(Strings.configure.purchase_instance_already_set.description) | ||
| } | ||
| #endif |
Member
There was a problem hiding this comment.
does that mean that any apps that have tests using our SDK will also crash here? If so we should find a way to limit this to just our testing, a customer should be able to do whatever they want in their tests
Contributor
Author
There was a problem hiding this comment.
Oh that's a fair point. But considering this is undesired usage of the SDK, and crashing during tests has no user impact, isn't that a good thing?
Running the SDK concurrently can lead to the concurrent post receipt operations issue.
Contributor
Author
There was a problem hiding this comment.
I updated this to use a separate environment variable
dc0acb0 to
3cff32b
Compare
Continuing to look into [CSDK-517], I found this several times in the logs: > Purchases instance already set. Did you mean to configure two Purchases objects? In order to ensure that tests don't leave around unwanted state, I've turned this error into an assertion _only when running tests_, as well as fixed those cases.
3cff32b to
b0631bf
Compare
NachoSoto
added a commit
that referenced
this pull request
Dec 12, 2022
Follow up to #2100. This has uncovered a huge source of leaks across many tests. Fixing this will provide many benefits: - Faster test runs - Lower memory overhead - Isolated state across each test run - Ensure that these types don't leak outside of tests too
NachoSoto
added a commit
that referenced
this pull request
Dec 13, 2022
Fixes #2107 and [SDKONCALL-180]. [This comment](#2107 (comment)) pointed out that parsing a long receipt can freeze the UI for almost a second.⚠️ This new assertion (running only in integration tests using the code added in #2100) has exposed a couple of code paths where we parse receipts from the main thread! [SDKONCALL-180]: https://revenuecats.atlassian.net/browse/SDKONCALL-180?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
This was referenced Dec 20, 2022
NachoSoto
added a commit
that referenced
this pull request
Dec 21, 2022
…tances in memory (#2180) We allow calling `Purchases.configure` multiple times. Likely a programmer error, but it's common in tests, or for example in `Purchase Tester` where users can re-configure the SDK at runtime (#1999), which is where I saw this issue. When this happens, the SDK was creating the new instance before clearing the old one from memory (clear thanks to #2082). Notice how the first `deinit` happens **after** the second `init`: ``` [Purchases] - VERBOSE: Purchases.init: created new Purchases instance: Purchases (0x0000600003ca4200) [Purchases] - INFO: ℹ️ Purchases instance already set. Did you mean to configure two Purchases objects? [Purchases] - VERBOSE: Purchases.init: created new Purchases instance: Purchases (0x0000600003c8a100) [Purchases] - VERBOSE: Purchases.deinit: Purchases (0x0000600003ca4200) [Purchases] - VERBOSE: Purchases.deinit: Purchases (0x0000600003c8a100) ``` With this change, we avoid that, which could also avoid issues with concurrent requests ([CSDK-517]): ``` [Purchases] - VERBOSE: Purchases.init: created new Purchases instance: Purchases (0x0000600002b6e600) [Purchases] - INFO: ℹ️ Purchases instance already set. Did you mean to configure two Purchases objects? [Purchases] - VERBOSE: Purchases.deinit: Purchases (0x0000600002b6e600) [Purchases] - VERBOSE: Purchases.init: created new Purchases instance: Purchases (0x0000600002b68100) [Purchases] - VERBOSE: Purchases.deinit: Purchases (0x0000600002b68100) ``` Unfortunately we don't have an easy way to test this, especially considering that we prohibit multiple instances in tests (#2100). [CSDK-517]: https://revenuecats.atlassian.net/browse/CSDK-517?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continuing to look into CSDK-517, I found this several times in the logs:
In order to ensure that tests don't leave around unwanted state, I've turned this error into an assertion only when running tests, as well as fixed those cases.