-
Notifications
You must be signed in to change notification settings - Fork 640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to junit5 and mockito #1550
Conversation
Rebased with buildx tests |
Codecov Report
@@ Coverage Diff @@
## master #1550 +/- ##
============================================
- Coverage 62.88% 62.64% -0.24%
+ Complexity 2155 2139 -16
============================================
Files 170 170
Lines 9894 9882 -12
Branches 1354 1354
============================================
- Hits 6222 6191 -31
- Misses 3159 3176 +17
- Partials 513 515 +2
|
OK, finally works on windows. |
Could you please review Sonar code smells and address whichever seem appropriate to you? |
All SQ issues removed from 'new' code. |
@rohanKanojia Ready to merge? |
Thanks, let me give it a quick review in a few days. |
result = "linux/amd64"; | ||
}}; | ||
Mockito.doReturn(buildXService).when(serviceHub).getBuildXService(); | ||
// Mockito.doReturn(null).when(authConfigFactory).createAuthConfig(false, false, null, null, null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excess mocking
Assertions.assertEquals("Response", response); | ||
verifyHttpClientExecute((HttpUriRequest request, ResponseHandler responseHandler) -> { | ||
Assertions.assertEquals(Collections.singletonMap("Accept", "*/*"), headersAsMap(request.getAllHeaders())); | ||
/* assertThat(responseHandler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this assertion commented? was it failing?
/* Mockito.verify(mockDelegate) | ||
.post(Mockito.anyString(), Mockito.isNull(), Mockito.anyMap(), Mockito.any(ResponseHandler.class), Mockito.eq(200)); | ||
|
||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already verified in method thenPushSucceeded()
result = Collections.emptyList(); | ||
}}; | ||
Mockito.doReturn(oldImageId, newImageId).when(queryService).getImageId(name); | ||
// Mockito.doReturn(Collections.emptyList()).when(docker).getImageTags("oldimage"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excess mocking. I have removed line
Kudos, SonarCloud Quality Gate passed! |
@chonton : Thanks a lot 💪 |
Mojo classes have many private fields changed to package-protected for unit tests to manipulate values.
junit updated from 4 to 5
jmockit migrated to mockito
multiple test frameworks removed in favor of junit5 capabilities