Static Mocking removal from ClevertapAPI #830
Conversation
- rather than nullable props with getters/setters - ready for conversion to access prop syntax kotlin - fixes test mock core state
…ests - fixes legacy tests which required mocking for controllermanager - does not reassign mock again - adds final properties in the class
…tate to kotlin - also does not allow access to integrator since it should not be allowed - makes changes to convert class to kotlin and make it internal
- adds CTExectors and getCoreState passed as a dependency in the constructor - uses internal constructor for test class so that we can pass mocked deps - fixes CleverTapAPITest.kt class to make tests leaner and avoid static mocking - cleans up CoreState.kt class to apply general kotlin idiomatic syntax. All of it is safe change suggested by IDE for access prop syntax and null un-wrapping suggestions, accidentally had defined val as var so fixes that as well
- Encapsulate time-related operations within a `Clock` interface and its `SYSTEM` implementation. - Replace direct calls to `System.currentTimeMillis()` and `Utils.getNow()` with `Clock` methods. - Update `CleverTapAPI` to accept a `Clock` instance for improved testability. - Modify tests to utilize a `TestClock` for controlled time scenarios in unit tests. - Remove `Utils.getNow()` as it's now handled by `Clock`.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes refactor core SDK initialization and execution logic by introducing a Kotlin-based Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Factory as CleverTapFactory
participant Core as CoreState
participant API as CleverTapAPI
participant Clock as Clock
App->>Factory: getCoreState(config)
Factory->>Core: Construct CoreState with dependencies
Factory-->>App: Return CoreState instance
App->>API: Initialize CleverTapAPI(context, config, coreState, clock)
API->>Core: Retrieve executors, managers, etc.
API->>Clock: Get current time (for session, etc.)
API->>API: asyncStartup() for async initialization
API-->>App: CleverTapAPI instance ready
Suggested labels
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt (2)
128-146: Async encryption-migration may clash with first DB reads
migrateEncryption()is kicked off viaexecutors.postAsyncSafelyTask()butgetCoreState()returns immediately.
Any component that starts reading encrypted prefs/DB rows throughcryptHandlerordatabaseManagerbefore this migration finishes can hit inconsistent state (e.g., mixed keys, partially-migrated rows).At minimum:
- Return a
Future/Jobfrom the task and await it in places that need consistent data, or- Run the migration synchronously during first creation (it’s a one-time cost), or
- Gate downstream reads behind a latch/flag set by the task.
Failing to do so risks hard-to-reproduce data-corruption bugs.
196-220: Race condition & non-thread-safe mutation ofStoreRegistry
storeRegistry.inAppStoreandimpressionStoreare mutated from a background IO task after the mutableStoreRegistryinstance has been published to the rest of the SDK (insideCoreState).
No synchronization/volatile barriers are present, so other threads may observe a half-initialised registry, leading toNullPointerExceptions when reading those fields.Consider:
- val storeRegistry = StoreRegistry(inAppStore = null, …) - executors.ioTask<Unit>().execute { storeRegistry.inAppStore = … } + // build fully initialised storeRegistry on background thread *before* publication + val storeRegistry = executors.ioTask<StoreRegistry>().submit { + StoreRegistry( + inAppStore = buildInAppStore(), + impressionStore = buildImpressionStore(), + … + ) + }.get() // or publish only after completionor make the fields inside
StoreRegistryimmutable and provide fully constructed instances up-front.
♻️ Duplicate comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java (1)
2109-2123:getTimeElapsed()still depends on static clockOnce the clock is made instance-scoped, remember to replace
clevertapClockhere as well.
🧹 Nitpick comments (12)
clevertap-core/src/main/java/com/clevertap/android/sdk/utils/Clock.kt (1)
50-56: Remove redundant overrides – rely on the interface defaults
currentTimeSeconds()andcurrentTimeSecondsInt()already have perfectly-good default implementations in theClockinterface.
Overriding them here just duplicates code and adds a maintenance surface without any behavioural gain.- override fun currentTimeSeconds(): Long { - return TimeUnit.MILLISECONDS.toSeconds(currentTimeMillis()) - } - - override fun currentTimeSecondsInt(): Int { - return (currentTimeMillis() / 1000).toInt() - }Dropping the two overrides keeps
SYSTEMlean and still exposes the exact same behaviour.clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapFactoryTest.kt (1)
43-46: Consider harmonising the camel-casing ofcTLockManagerThe new property uses a capital “T” in the middle (
cTLockManager) while most SDK fields follow lower-camel (ctLockManager).
Purely cosmetic, but worth aligning for searchability and consistency.clevertap-core/src/test/java/com/clevertap/android/sdk/AnalyticsManagerTest.kt (1)
90-92: Same naming-style nit as above
cTLockManagerbreaks the usual lower-camel convention. If possible, rename toctLockManageracross prod & test code for uniformity.clevertap-core/src/test/java/com/clevertap/android/sdk/EventQueueManagerTest.kt (1)
65-66: Camel-case inconsistency forcTLockManagerSee earlier comments – consider renaming to
ctLockManagerto follow the prevailing style.clevertap-core/src/test/java/com/clevertap/android/sdk/MockCoreState.kt (1)
7-38: Constructor call is getting unwieldy – consider named arguments or a builderPassing 30-plus positional parameters makes the test double fragile to future signature tweaks and hard to scan.
Using named arguments (Kotlin) or a small builder/helper would greatly improve readability:MockCoreStateKotlin( config = cleverTapInstanceConfig, coreMetaData = CoreMetaData(), deviceInfo = mockk(relaxed = true), // … )Not urgent, but worth refactoring before the parameter list grows again.
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt (1)
259-262: CTVariables initialisation fire-and-forget
ctVariables.init()may throw (reflection, IO) but the lambda result is ignored; failures silently disappear.
Recommend adding try/catch with explicit logging or returning aResult/Futureto surface errors for troubleshooting.clevertap-core/src/main/java/com/clevertap/android/sdk/CoreState.kt (2)
25-57: MakeCoreStatenon-openunless subclassing is requiredThe class only serves as a dependency container; allowing inheritance widens the surface for improper extension and breaks immutability assumptions.
Mark itfinal(default in Kotlin) unless there is a concrete need for testing/mocking.
64-68: Deprecated accessor still performs work
getCtProductConfigController()delegates toinitProductConfig()which may allocate heavy objects and mutate shared state even though the API is deprecated.
Either:• Remove side-effects and simply return the cached controller, or
• Hard-deprecate by throwingUnsupportedOperationExceptionto prevent new usage.This helps phase-out the API cleanly.
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java (1)
1239-1280:asyncStartup()– missing exception handling inside tasksIf any of the lambdas throws, the task will silently swallow the exception (it’s executed inside an executor). Consider wrapping each callable with try/catch and logging, or use a helper that does this consistently so startup failures are visible.
task.execute("CleverTapAPI#initializeDeviceInfo", () -> { + try { ... - return null; + } catch (Throwable t) { + getConfigLogger().error(getAccountId(), "Startup task failed", t); + } + return null; });clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapAPITest.kt (3)
62-76: Use descriptive constants instead of magic numbers for clock manipulation
testClock.setCurrentTime(Int.MAX_VALUE.toLong())relies on the reader knowing the time unit and the 5-second threshold logic in production code. Replacing the literal with a named constant (e.g.,MORE_THAN_FIVE_SECONDS) or usingTimeUnit.SECONDS.toMillis(10)would improve intent clarity and future-proof the assertion.
375-376: Property-access verification is brittle
verify(exactly = 2) { corestate.controllerManager.ctInboxController }counts the number of property reads. Any future refactor that introduces an extra read (e.g., logging) will break the test without changing behaviour.Prefer verifying the interaction on the real collaborator:
verify { inboxController.deleteInboxMessagesForIDs(messageIDs) }and drop the property-access count, or wrap the property in a local variable inside production code and verify downstream calls instead.
354-361:confirmVerifiedimmediately afterverifyoffers little valueThe pattern
verify { corestate.controllerManager.ctInboxController } confirmVerified(corestate.controllerManager)adds minimal assertion power because only a single interaction is expected. It can be removed or replaced with a more explicit interaction assertion (e.g.,
verify(exactly = 1)), reducing noise and improving readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java(22 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt(8 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/CoreState.java(0 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/CoreState.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/Utils.java(0 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/utils/Clock.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/AnalyticsManagerTest.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapAPITest.kt(9 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapFactoryTest.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/EventQueueManagerTest.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/MockCoreState.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/UtilsTest.kt(0 hunks)
💤 Files with no reviewable changes (3)
- clevertap-core/src/test/java/com/clevertap/android/sdk/UtilsTest.kt
- clevertap-core/src/main/java/com/clevertap/android/sdk/CoreState.java
- clevertap-core/src/main/java/com/clevertap/android/sdk/Utils.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt (1)
Learnt from: vasct
PR: CleverTap/clevertap-android-sdk#797
File: clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTInboxActivity.java:89-92
Timestamp: 2025-04-28T09:52:18.094Z
Learning: In the CleverTap Android SDK, if a CleverTapAPI instance is not null, then its getCoreState() method will never return null because CoreState is initialized in the constructor and set via setCoreState(). The CleverTapFactory.getCoreState() method throws a RuntimeException if it can't create a valid state.
clevertap-core/src/main/java/com/clevertap/android/sdk/CoreState.kt (1)
Learnt from: vasct
PR: CleverTap/clevertap-android-sdk#797
File: clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTInboxActivity.java:89-92
Timestamp: 2025-04-28T09:52:18.094Z
Learning: In the CleverTap Android SDK, if a CleverTapAPI instance is not null, then its getCoreState() method will never return null because CoreState is initialized in the constructor and set via setCoreState(). The CleverTapFactory.getCoreState() method throws a RuntimeException if it can't create a valid state.
🧬 Code Graph Analysis (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/variables/Parser.java (1)
Parser(25-195)
🔇 Additional comments (4)
clevertap-core/src/test/java/com/clevertap/android/sdk/EventQueueManagerTest.kt (1)
63-63: Good change – explicit cast removedPassing
corestate.networkManagerdirectly keeps the constructor call clean and avoids an unnecessary cast.clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt (1)
460-491:CoreStateadvertised as immutable but contains mutable sub-objectsAlthough the new
CoreStateis built withvalproperties, several references (storeRegistry,controllerManager, etc.) are mutated later by background tasks.
Document this clearly or enforce immutability by:
- Initialising these collaborators completely before construction, or
- Wrapping mutable fields in thread-safe holders.
Without this, the “immutable container” guarantee is misleading to maintainers.
clevertap-core/src/main/java/com/clevertap/android/sdk/CoreState.kt (1)
77-98: Possible NPE whencontextis null
initProductConfig(null)passes a nullable context down toCTProductConfigFactory.getInstance(...).
If that factory does not explicitly handlenull, an NPE will surface.
Guard early:if (context == null) returnor enforce a non-null contract.
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java (1)
1009-1014: First-foreground timestamp can race
clevertapClock.currentTimeSecondsInt()is read without synchronisation while other threads may setCoreMetaData.setInitialAppEnteredForegroundTime(...).
If you move to an instance-scoped clock, also guard this block or migrate the field toAtomicIntegerto avoid lost updates in multi-threaded apps.
| MockCTExecutors() | ||
| ) |
There was a problem hiding this comment.
MockCTExecutors() is missing the required config argument – compilation will fail
All other usages instantiate the helper with the CleverTapInstanceConfig parameter:
MockCTExecutors(cleverTapInstanceConfig)Apply the same here to satisfy the constructor.
- MockCTExecutors()
+ MockCTExecutors(cleverTapInstanceConfig)🤖 Prompt for AI Agents
In clevertap-core/src/test/java/com/clevertap/android/sdk/MockCoreState.kt at
lines 37 to 38, the instantiation of MockCTExecutors is missing the required
CleverTapInstanceConfig argument. Fix this by passing the
cleverTapInstanceConfig parameter to the MockCTExecutors constructor, matching
other usages to ensure successful compilation.
| val taskInitFCManager = executors.ioTask<Unit>() | ||
| taskInitFCManager.execute("initFCManager") { | ||
| if (coreState.deviceInfo != null && coreState.deviceInfo.deviceID != null && controllerManager.inAppFCManager == null) { | ||
| coreState.config.logger | ||
| val deviceId = deviceInfo.deviceID | ||
| if (deviceId != null && controllerManager.inAppFCManager == null) { | ||
| config.logger | ||
| .verbose( | ||
| config.accountId + ":async_deviceID", | ||
| "Initializing InAppFC with device Id = " + coreState.deviceInfo | ||
| .deviceID | ||
| "Initializing InAppFC with device Id = $deviceId" | ||
| ) | ||
| controllerManager.inAppFCManager = InAppFCManager( | ||
| context, | ||
| config, | ||
| coreState.deviceInfo.deviceID, | ||
| deviceId, | ||
| storeRegistry, | ||
| impressionManager | ||
| ) | ||
| } | ||
| null | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Async FC-manager initialisation can leave inAppFCManager null
initFCManager runs off-thread and populates controllerManager.inAppFCManager.
Callers that access this field immediately after obtaining CoreState can still find it null. Either:
• Make ControllerManager.inAppFCManager lazy & thread-safe, or
• Block until the task finishes, or
• Expose a CompletableFuture that callers can await.
This prevents sporadic NullPointerExceptions during early app-startup analytics.
🤖 Prompt for AI Agents
In clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt
around lines 223 to 240, the asynchronous initialization of inAppFCManager can
cause it to be null when accessed immediately after CoreState creation, leading
to NullPointerExceptions. To fix this, modify the initialization to either make
inAppFCManager lazy and thread-safe, or block the calling thread until the
initFCManager task completes, or alternatively expose a CompletableFuture that
callers can await to ensure initialization is complete before access.
| private static Clock clevertapClock = Clock.SYSTEM; | ||
|
|
There was a problem hiding this comment.
Static mutable clock breaks multi-instance safety
clevertapClock is static, yet it is mutated in every constructor.
With multiple SDK instances (or tests that inject a fake clock) the last-created instance overwrites the clock for all others, leading to time-skew bugs and data races.
- private static Clock clevertapClock = Clock.SYSTEM;
+ private final Clock clock;and update all clevertapClock.* calls to this.clock.* (or expose it through coreState).
This keeps the dependency truly instance-scoped.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java
around lines 168 to 169, the static mutable field clevertapClock is shared
across all instances, causing multi-instance safety issues. Change
clevertapClock from static to an instance field, initialize it per instance in
the constructor, and update all references from clevertapClock.* to this.clock.*
or access it via the instance's coreState to ensure the clock is instance-scoped
and avoid data races.
There was a problem hiding this comment.
its okay if it mutates; we always use system clock.
There was a problem hiding this comment.
@CTLalit Understood! Since you always use the system clock across all instances, the shared static field won't cause practical issues in your use case. I'll note this design decision for future reference.
✏️ Learnings added
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#830
File: clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java:168-169
Timestamp: 2025-06-17T13:54:08.118Z
Learning: In the CleverTap Android SDK, the static mutable clevertapClock field in CleverTapAPI.java is acceptable because all instances consistently use Clock.SYSTEM, so the mutation doesn't cause practical multi-instance issues in their use case.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: vasct
PR: CleverTap/clevertap-android-sdk#797
File: clevertap-core/src/main/java/com/clevertap/android/sdk/inbox/CTInboxActivity.java:89-92
Timestamp: 2025-04-28T09:52:18.094Z
Learning: In the CleverTap Android SDK, if a CleverTapAPI instance is not null, then its getCoreState() method will never return null because CoreState is initialized in the constructor and set via setCoreState(). The CleverTapFactory.getCoreState() method throws a RuntimeException if it can't create a valid state.
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
| // Initialize | ||
| private CleverTapAPI(final Context context, final CleverTapInstanceConfig config, String cleverTapID) { | ||
| this.context = context; | ||
| private CleverTapAPI( | ||
| final Context context, | ||
| final CleverTapInstanceConfig config, | ||
| String cleverTapID | ||
| ) { | ||
| this(context, config, CleverTapFactory.getCoreState(context, config, cleverTapID), Clock.SYSTEM); | ||
| } | ||
|
|
||
| CoreState coreState = CleverTapFactory.getCoreState(context, config, cleverTapID); | ||
| setCoreState(coreState); | ||
| // Internal constructor for testing | ||
| CleverTapAPI( | ||
| final Context context, | ||
| final CleverTapInstanceConfig config, | ||
| final CoreState coreState, | ||
| final Clock clock | ||
| ) { | ||
| this.context = context; | ||
| this.coreState = coreState; | ||
| CleverTapAPI.clevertapClock = clock; | ||
| getConfigLogger().verbose(config.getAccountId() + ":async_deviceID", "CoreState is set"); | ||
| asyncStartup(); | ||
| Logger.i("CleverTap SDK initialized with accountId: " + config.getAccountId() + " accountToken: " + config | ||
| .getAccountToken() + " accountRegion: " + config.getAccountRegion()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constructor ignores injected clock
You pass a Clock but immediately stash it into the static field, losing the benefit of injection and re-introducing the global mutable state.
- CleverTapAPI.clevertapClock = clock;
+ this.clock = clock; // instance fieldDo not mutate global state here; retain the injected dependency per instance.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapAPI.java
between lines 1214 and 1237, the constructor assigns the injected Clock
parameter to a static field, causing global mutable state and ignoring the
benefit of dependency injection. To fix this, remove the assignment to the
static field and instead store the Clock instance in a non-static instance
variable so each CleverTapAPI object retains its own injected Clock without
affecting global state.
There was a problem hiding this comment.
@CTLalit Thanks for addressing the dependency injection issue! Storing the Clock as an instance variable instead of assigning it to the static field is the right approach for proper testability and avoiding global mutable state.
🧠 Learnings used
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @Test | ||
| fun testCleverTapAPI_constructor_when_InitialAppEnteredForegroundTime_greater_than_5_secs() { | ||
| executeBasicTest { | ||
| mockkStatic(Utils::class) { | ||
| // Arrange | ||
| every { Utils.getNow() } returns Int.MAX_VALUE | ||
| CoreMetaData.setInitialAppEnteredForegroundTime(0) | ||
|
|
||
| // Act | ||
| initializeCleverTapAPI() | ||
|
|
||
| // Assert | ||
| assertTrue( | ||
| "isCreatedPostAppLaunch must be true", | ||
| cleverTapInstanceConfig.isCreatedPostAppLaunch | ||
| ) | ||
| verifyCommonConstructorBehavior() | ||
| } | ||
| } | ||
| @Ignore("This might be actual bug which happens due to long to int conversion, current ts does not overflow and cause this test to fail in real device") | ||
| fun testCleverTapAPI_constructor_when_InitialAppEnteredForegroundTime_greater_than_5_secs_long_int_conversion() { | ||
| testClock.setCurrentTime(Long.MAX_VALUE) | ||
| CoreMetaData.setInitialAppEnteredForegroundTime(0) | ||
|
|
||
| // Act | ||
| initializeCleverTapAPI() | ||
|
|
||
| // Assert | ||
| assertTrue( | ||
| "isCreatedPostAppLaunch must be true", | ||
| cleverTapInstanceConfig.isCreatedPostAppLaunch | ||
| ) | ||
| verifyCommonConstructorBehavior() | ||
| } |
There was a problem hiding this comment.
Ignored test hides a production bug – prefer investigation or Assume over @Ignore
Marking testCleverTapAPI_constructor_when_…_long_int_conversion with @Ignore permanently suppresses coverage of a potential overflow defect noted in the comment. This masks regressions.
If the overflow only affects certain ABIs/SDKs, gate the test with an Assume.assumeTrue(...) check or parameterised test instead of ignoring it outright, and file/track a ticket referencing the root cause.
🤖 Prompt for AI Agents
In clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapAPITest.kt
around lines 78 to 93, the test is currently ignored with @Ignore which hides a
potential overflow bug. Remove the @Ignore annotation and instead use an
Assume.assumeTrue(...) check to conditionally skip the test only on affected
ABIs or SDK versions. This way, the test runs in relevant environments and the
bug remains visible. Also, create and link a ticket to track the root cause of
the overflow issue.
| private fun initializeCleverTapAPI() { | ||
| cleverTapAPI = CleverTapAPI(application, cleverTapInstanceConfig, corestate, testClock) | ||
|
|
||
| private fun executeBasicTestWithAny(block: () -> Unit) { | ||
| executeMockExecutors { | ||
| executeMockFactoryWithAny { | ||
| block() | ||
| } | ||
| } | ||
| // we need to do this for static methods of CleverTapAPI tests to work correctly. | ||
| CleverTapAPI.setInstances(hashMapOf(Constant.ACC_ID to cleverTapAPI)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Static instance map manipulation may lead to subtle test coupling
initializeCleverTapAPI() pushes a new entry into the global CleverTapAPI.instances map on every invocation.
When a test method calls this helper more than once (or when future tests add calls without noticing), the previous entry remains unless explicitly cleared, causing shared state across tests and order-dependent failures.
Consider scoping the map population strictly to the JUnit lifecycle:
@@
-private fun initializeCleverTapAPI() {
- cleverTapAPI = CleverTapAPI(application, cleverTapInstanceConfig, corestate, testClock)
-
- // we need to do this for static methods of CleverTapAPI tests to work correctly.
- CleverTapAPI.setInstances(hashMapOf(Constant.ACC_ID to cleverTapAPI))
-}
+private fun initializeCleverTapAPI() {
+ cleverTapAPI = CleverTapAPI(application, cleverTapInstanceConfig, corestate, testClock)
+ CleverTapAPI.setInstances(mutableMapOf(Constant.ACC_ID to cleverTapAPI))
+}
+
+@Before
+fun beforeEach() {
+ CleverTapAPI.setInstances(null) // clear any leftovers
+ ...
+}
+
+@After
+fun afterEach() {
+ CleverTapAPI.setInstances(null) // guarantee isolation
+}This keeps tests isolated and avoids unintended leakage.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapAPITest.kt
around lines 34 to 39, the initializeCleverTapAPI method adds entries to the
global CleverTapAPI.instances map without clearing it, causing shared state
across tests. To fix this, clear or reset the instances map before or after each
test using JUnit lifecycle methods such as @Before or @After to ensure test
isolation and prevent order-dependent failures.
There was a problem hiding this comment.
fixed in commit d37a3c8
There was a problem hiding this comment.
@CTLalit Thank you for addressing the test isolation concern! The fix ensures that tests won't interfere with each other through shared static state, which will improve test reliability and maintainability.
🧠 Learnings used
Learnt from: CTLalit
PR: CleverTap/clevertap-android-sdk#798
File: clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt:152-152
Timestamp: 2025-05-30T08:21:16.962Z
Learning: CTLalit prefers to avoid modifying legacy code when working on specific features, keeping the scope focused to reduce risk and maintain clear boundaries for changes.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
https://wizrocket.atlassian.net/browse/SDK-4921
remove static mocking for CTExecutors
CoreState.java → convert to kotlin for configurability
CtExecutors → pass as a dependency so we can use MockCtExecutors in test
Configurable clock → Rather than current time in millis which makes it hard to test
Summary by CodeRabbit