[Test Improver] Add unit tests for ConnectOpportunitiesParser#3632
[Test Improver] Add unit tests for ConnectOpportunitiesParser#3632github-actions[bot] wants to merge 3 commits intomasterfrom
Conversation
Tests cover all branches of parse(): empty body guard, valid job parsing, corrupt job handling (JSONException path), new-jobs gate for scheduleOneTimeFetch, and the outer JSONException throw path. Static methods (ConnectJobUtils, FirebaseAnalyticsUtil, ConnectJobRecord) mocked with MockK mockkStatic; Companion object mocked with mockkObject. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
||
| @Test | ||
| fun testEmptyResponseBody_returnsEmptyModel_noStoreJobsCalled() { | ||
| val inputStream: InputStream = ByteArrayInputStream("".toByteArray()) |
There was a problem hiding this comment.
It's not necessary to add the InputStream type here. Remove the explicit type. The same goes for the rest of the class because this is repeated multiple times.
| @Test | ||
| fun testEmptyJsonArray_returnsEmptyModel_storeJobsCalledOnce() { | ||
| val inputStream: InputStream = ByteArrayInputStream(JSONArray().toString().toByteArray()) | ||
| every { ConnectJobUtils.storeJobs(any(), any(), any()) } returns 0 | ||
| every { FirebaseAnalyticsUtil.reportCccApiJobs(any(), any(), any()) } just Runs | ||
|
|
||
| val result = parser.parse(200, inputStream, context) | ||
|
|
||
| assertEquals(0, result.validJobs.size) | ||
| assertEquals(0, result.corruptJobs.size) | ||
| verify(exactly = 1) { ConnectJobUtils.storeJobs(context, any(), true) } | ||
| } |
There was a problem hiding this comment.
If the json array is empty, we probably should not call storeJobs(). Perhaps we should tweak the original function to not call storeJobs() if there are no jobs to store.
| @Test | ||
| fun testSingleValidJob_parsedJobAddedToValidJobsList() { | ||
| val mockJob = mockk<ConnectJobRecord>() | ||
| val array = JSONArray().apply { put(JSONObject().apply { put("id", 42) }) } | ||
| val inputStream: InputStream = ByteArrayInputStream(array.toString().toByteArray()) | ||
| every { ConnectJobRecord.fromJson(any()) } returns mockJob | ||
| every { ConnectJobUtils.storeJobs(any(), any(), any()) } returns 0 | ||
| every { FirebaseAnalyticsUtil.reportCccApiJobs(any(), any(), any()) } just Runs | ||
|
|
||
| val result = parser.parse(200, inputStream, context) | ||
|
|
||
| assertEquals(1, result.validJobs.size) | ||
| assertEquals(0, result.corruptJobs.size) | ||
| } |
There was a problem hiding this comment.
We need to change this test. Actually, maybe we need a new approach to this entire test class. We expect ConnectJobUtils.storeJobs() to return the number of jobs that have been added. So, ideally, we should be verifying that ConnectJobUtils.storeJobs() returns exactly 1 in this scenario. Also, while we should mock the database transaction itself, we should somehow create some dummy data for the job/opportunity, use that in the tests, and verify that the valid jobs spit out of parser.parse() match the dummy data. In other words, we are mocking too much here.
…icit types - Remove explicit InputStream type annotations throughout test class - Guard ConnectJobUtils.storeJobs call: skip when jobs list is empty - Rework testSingleValidJob to use real JSON and verify parsed job data - Add validJobJson/appJson helpers for constructing complete job JSON - Rename testEmptyJsonArray to reflect storeJobs is no longer called 🤖 PR Comment Handler
|
Commit pushed:
|
|
🤖 PR Comment Handler here — I've processed the latest review feedback. Implemented:
Please re-review the latest commit. Happy to make further adjustments.
|
Move ConnectJobRecord mocking out of setUp/tearDown and into only the test that requires it (testCorruptJobEntry). All other tests now use real JSON parsing via the validJobJson helper, removing unnecessary static mocking throughout the class. 🤖 PR Comment Handler
|
Commit pushed:
|
|
🤖 PR Comment Handler here — I've processed the latest review feedback. Implemented:
Please re-review the latest commit. Happy to make further adjustments.
|
1 participant
🤖 Test Improver — automated AI assistant
Goal and Rationale
ConnectOpportunitiesParserparses the full job opportunity list from the Connect API and had zero test coverage. It contains multiple meaningful branches:JSONExceptionhandling (corrupt job path viahandleCorruptJob)newJobs > 0gate that controls whetherConnectReleaseTogglesWorker.scheduleOneTimeFetchis calledJSONExceptioncatch that re-throws asRuntimeExceptionand reports failure to FirebaseWhy it matters: This parser controls which opportunities appear in the worker's job list. Silent bugs in any of these branches would either lose valid jobs, silently ignore corrupt entries, or miss triggering the feature-flag refresh.
Approach
New test class:
app/unit-tests/src/org/commcare/connect/network/connect/parser/ConnectOpportunitiesParserTest.ktTests cover the complete
ConnectOpportunitiesParser.parse()method:testEmptyResponseBody_returnsEmptyModel_noStoreJobsCalledstoreJobsnever calledtestEmptyJsonArray_returnsEmptyModel_storeJobsCalledOnce[]→ both lists empty,storeJobscalled oncetestSingleValidJob_parsedJobAddedToValidJobsListvalidJobshas 1 item,corruptJobsemptytestMultipleValidJobs_allAddedToValidJobsListvalidJobshas 2 itemstestCorruptJobEntry_fromJsonThrowsJSONException_addsToCorruptJobsListfromJsonthrowsJSONException→ entry goes tocorruptJobs,getName()assertedtestNewJobsGreaterThanZero_schedulesOneTimeFetchstoreJobsreturns 2 →scheduleOneTimeFetch(context)called exactly oncetestNewJobsEqualsZero_doesNotScheduleOneTimeFetchstoreJobsreturns 0 →scheduleOneTimeFetchnever calledtestParseSuccess_reportsApiJobsWithCorrectCountsreportCccApiJobs(true, 1, 3)asserted with specific valuestestInvalidJson_throwsRuntimeExceptionJSONException→RuntimeExceptionthrowntestInvalidJson_reportsApiCallWithFailurereportCccApiJobs(false, 0, 0)calledMock strategy:
ConnectJobRecord.fromJson/corruptJobFromJson—mockkStatic(ConnectJobRecord::class)(Java static)ConnectJobUtils.storeJobs—mockkStatic(ConnectJobUtils::class)(Java static)FirebaseAnalyticsUtil.reportCccApiJobs—mockkStatic(FirebaseAnalyticsUtil::class)(Java static)ConnectReleaseTogglesWorker.scheduleOneTimeFetch—mockkObject(ConnectReleaseTogglesWorker.Companion)(Kotlin companion object, not@JvmStatic)This avoids constructing the complex nested JSON required by
ConnectJobRecord.fromJson()(which depends onDateUtils.parseDate,ConnectAppRecord.fromJson,ConnectPaymentUnitRecord.fromJson, andConnectLearnModuleSummaryRecord.fromJson), keeping tests focused on the parser's own branching logic.Coverage Impact
ConnectOpportunitiesParser.ktpreviously had 0% coverage. This PR covers all branches ofparse():JSONExceptioncatch →handleCorruptJobnewJobs > 0gateJSONExceptioncatch pathTrade-offs
ConnectJobRecord.fromJsonis mocked rather than exercised with real JSON — intentional, sincefromJsonbelongs toConnectJobRecord, not this parser.handleCorruptJob's innerJSONExceptionpath (whencorruptJobFromJsonitself throws) is not covered — it's a secondary error-handling path with no observable side effect beyond a log call.Test Status
Build not runnable locally (requires
../commcare-core/sibling, checked out only in CI).ktlintFileunavailable in this environment (Gradle wrapper lock — infrastructure constraint). File follows the same import ordering and annotation style asPushNotificationHelperTest.kt(the existing MockK test in the suite).To run: