Voice API by generator#307
Voice API by generator#307JPPortier wants to merge 3 commits intoDEVEXP-1002-verification-api-by-generatorfrom
Conversation
JPPortier
requested review from
asein-sinch,
matsk-sinch and
rpredescu-sinch
as code owners
Dependency Review✅ No vulnerabilities or license issues found.Scanned Manifest Files |
| .thenReturn(httpResponse); | ||
|
|
||
| service.updateCallbackUrls("app/key", UpdateCallbackUrlsRequestTest.expected); | ||
| } |
There was a problem hiding this comment.
Hi, @JPPortier!
This test might be missing an assertion to verify that the mocked HTTP call was actually made. Currently it only sets up the mock behavior with when(), but doesn't confirm the service actually invokes it. The test will pass even if the service method doesn't call httpClient.invokeAPI() at all, since there's no explicit verification, isn't it?
Should we add this kind of verification at the end? of the method?
verify(httpClient).invokeAPI(
eq(serverConfiguration),
eq(authManagers),
argThat(new HttpRequestMatcher(httpRequest)));
The same pattern can be applied to updateNumbers() and unassignNumber() tests.
What do you think?
There was a problem hiding this comment.
No: framework used for validation will trigger an error if the mocked request is not used.
Then here the test will fail if we fail to setup the proper mock to be managed by the request -> ie the proper URL/parameter to be sent.
The proposal you made is exactly covered by
when(httpClient.invokeAPI(
eq(serverConfiguration),
eq(authManagers),
argThat(new HttpRequestMatcher(httpRequest))))
.thenReturn(httpResponse);and will throw org.mockito.exceptions.misusing.PotentialStubbingProblem if not processed
| .thenReturn(httpResponse); | ||
|
|
||
| service.update("call/id", SvamlControlTest.expectedSvamlControl); | ||
| } |
There was a problem hiding this comment.
Hi @JPPortier!
Same observation as for #307 (comment): update() and manageWithCallLeg() could introduce a verify statement at the end?
Best!
There was a problem hiding this comment.
(see comment onto ApplicationServiceTest)
| parameter = conferenceIdCaptor.getValue(); | ||
| Assertions.assertThat(parameter) | ||
| .isEqualTo(CalloutRequestDtoTest.conferenceRequestCalloutDto.getConferenceId()); | ||
| } |
There was a problem hiding this comment.
Hi @JPPortier!
Same potentially missing verify() for:
- kickParticipant() - missing verify
- kickAll() - missing verify
- manageParticipant() - missing verify
See: #307 (comment).
Thanks!
There was a problem hiding this comment.
(see comment onto ApplicationServiceTest)
2 participants
No description provided.