feat(SDK-4171): Make Analytics manager testable #689
Conversation
CTLalit
commented
Nov 14, 2024
CTLalit
commented
- removes context passed in Clevertap instance config
- breaks down code to separate out context
- uses context held in the api wrapper and not holding another ref in class
- breaking down incorrect static usages
- private constructor to init things in test class
- provides control over clevertap instance config and also manifest info
- tests can now cover manifest info testing
- fixes singleton access for manifestinfo (sets stage)
- moves manifest constants to correct class -> breaks down large constants class
- creates clevertap fixtures which will replace base test case class which provides unpredictable mocks and incomplete tests.
- removes context passed in Clevertap instance config - breaks down code to separate out context - uses context held in the api wrapper and not holding another ref in class - breaking down incorrect static usages - private constructor to init things in test class - provides control over clevertap instance config and also manifest info - tests can now cover manifest info testing - fixes singleton access for manifestinfo (sets stage) - moves manifest constants to correct class -> breaks down large constants class - creates clevertap fixtures which will replace base test case class which provides unpredictable mocks and incomplete tests.
CTLalit
requested review from
Anush-Shand,
piyush-kukadiya,
darshanclevertap and
vasct
- cases where ct does not process pn added
- migrates from mockito to mockk - extracts out current time ms func, this is to gain control over clock and testing - changes test cases to mockk - adds a new test for duplicate PN not rendering logic
- compares json within method which gets generated correctly.
- this is to emulate the current time
- for a seamless test code so we can avoid replication
- adds notification clicked events.
- failure cases when sdk is in analytics mode - notifications not from clevertap - comes with incorrect params.
| } catch (Throwable t) { | ||
| // no-op | ||
| } | ||
| coreMetaData.setWzrkParams(AnalyticsManagerBundler.INSTANCE.wzrkBundleToJson(extras)); |
There was a problem hiding this comment.
wzrkBundleToJson throws and the exception seems unhandled now. Is possible make the method not throwing or try-catch here.
| object AnalyticsManagerBundler { | ||
|
|
||
| @Throws(JSONException::class) | ||
| fun wzrkBundleToJson(root: Bundle): JSONObject { |
There was a problem hiding this comment.
You can add @JvmStatic annotation to both methods in this object. So when calling them from java, they can be called form the class instead of the INSTANCE.
|
|
||
| private final HashMap<String, Object> notificationIdTagMap = new HashMap<>(); | ||
|
|
||
| private final Function0<Long> currentTimeProvider; |
There was a problem hiding this comment.
You can use the com.clevertap.android.sdk.utils.Clock interface which looks like it was created for this purpose. Or usejava.time.Clock
|
|
||
| // convenience to construct the internal only default config | ||
| @SuppressWarnings({"unused", "WeakerAccess"}) | ||
| protected static CleverTapInstanceConfig createDefaultInstance( |
There was a problem hiding this comment.
The access modifiers to this method and createInstanceWithManifest are a bit weird. This is protected, but the other is public and can achieve the same thing.
| @@ -111,8 +110,7 @@ public void setParser(final Parser parser) { | |||
| this.parser = parser; | |||
| } | |||
|
|
|||
| CoreState(final Context context) { | |||
| this.context = context; | |||
| CoreState() { | |||
There was a problem hiding this comment.
No need for this constructor
| * | ||
| * Should be singleton and initialised only once -> need to validate. | ||
| */ | ||
| // todo lp Remove context dependency from here | ||
| @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
| public class ManifestInfo { |
There was a problem hiding this comment.
You've changed a lot of code in this class, consider converting it to Kotlin. I think it would benefit the readability.
| import org.mockito.* | ||
|
|
||
| class MockCoreState(context: Context, cleverTapInstanceConfig: CleverTapInstanceConfig) : CoreState(context) { | ||
| // todo lp check usages and eliminate context setup | ||
| class MockCoreState(cleverTapInstanceConfig: CleverTapInstanceConfig) : CoreState() { |
There was a problem hiding this comment.
All of the test classes that use MockCoreState are Kotlin classes. Could we remove this implementation and use only the one in MockCoreStateKotlin ?
| ) | ||
| } | ||
|
|
||
| // setup again, 10000 ms has passed |
There was a problem hiding this comment.
The difference is 2000ms from the initial mock. Not sure if it should be 200ms based on the test name, but it is not 10000ms
| } | ||
|
|
||
| // setup again, 10000 ms has passed | ||
| every { timeProvider.invoke() } returns 20000 |
There was a problem hiding this comment.
I think it would be better to test with the minimum amount that should pass the test 15000 or 15001
| assertEquals(512, captor.value.errorCode) | ||
| verify { | ||
| validationResultStack.pushValidationResult(withArg { arg -> | ||
| assertEquals(512, arg.errorCode) |
There was a problem hiding this comment.
Do we have a constant for this and other error codes used in this test class?
