Conversation
| import com.revenuecat.purchases.google.BillingWrapper | ||
|
|
||
| @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) | ||
| @VisibleForTesting(otherwise = VisibleForTesting.PACKAGE_PRIVATE) |
There was a problem hiding this comment.
I checked this and the only usage of this method outside this module is on the amazon features module to run a test. However, I think that test could (and probably should) be moved to the purchases module instead, which would allow to make this internal which is much preferred since it would avoid devs from using this at all. I didn't do this just yet in case I missed something, but that should be a quick change.
| * @warning Only one instance of Purchases should be instantiated at a time! | ||
| */ | ||
| @Suppress("LongParameterList") | ||
| class Purchases @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) internal constructor( |
There was a problem hiding this comment.
Moving the constructor logic outside means that this constructor needs to be internal now
| proxyURL | ||
| ).also { | ||
| @SuppressLint("RestrictedApi") | ||
| sharedInstance = it |
There was a problem hiding this comment.
This remains not tested but the logic here now is minimal. I thought about creating another configure method with a PurchasesFactory parameter so we could add tests to that, but we would have to make it visible for testing, which in turn would allow devs access to it. Considering the value of the tests here (where there is almost no logic), I thought it wasn't needed but lmk if you think otherwise
There was a problem hiding this comment.
Still better than what we had before so I think it's fine!
|
|
||
| private fun Context.hasPermission(permission: String): Boolean { | ||
| return checkCallingOrSelfPermission(permission) == PackageManager.PERMISSION_GRANTED | ||
| } |
There was a problem hiding this comment.
I thought about moving these to the utils.kt file in the common module, but they are only used here, so I kept them private for now.
| fun setup() { | ||
| purchasesFactory = PurchasesFactory() | ||
|
|
||
| clearAllMocks() |
There was a problem hiding this comment.
I noticed we have some tests where we don't seem to be clearing the mocks between tests, like the tests in PurchasesTest. All mocks should be isolated between tests. I added a ticket here to take a look into this: https://revenuecats.atlassian.net/browse/CSDK-111
| dangerousSettings | ||
| ) | ||
|
|
||
| val prefs = PreferenceManager.getDefaultSharedPreferences(application) |
There was a problem hiding this comment.
To be able to test this more thoroughly, we would need to create factories for each of these dependencies. Considering there is not much more logic here, I thought the value of doing that now wasn't that big, but lmk if you think otherwise or have any other ideas.
vegaro
left a comment
There was a problem hiding this comment.
LGTM! This was very much needed
| dangerousSettings | ||
| ) | ||
|
|
||
| val prefs = PreferenceManager.getDefaultSharedPreferences(application) |
Description
https://revenuecats.atlassian.net/browse/CSDK-101
This PR moves the creation of the
Purchasesobject to a factory class. This allows us to test the validation logic that happens during this process. In the future, it might be good to test the whole process, but for that, we will need to create factories for the different objects that are created during this initialization process which might take a bit longer. I judged the value of that not to be that big considering there is no more logic on that part of the code aside from the validations which are already tested with these changes.